Closed Bug 1114515 Opened 10 years ago Closed 10 years ago

[Bluetooth2] Implement GATT service discovery API

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

Product:
Firefox OS Graveyard
Component:
Bluetooth
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox39 fixed)

Status:
RESOLVED FIXED
Milestone:
2.2 S9 (3apr)
Tracking Flags:
Tracking Status
firefox39 --- fixed

People

(Reporter: yrliou, Assigned: yrliou)

References

Details

(Whiteboard: [webbt-api])

Attachments

(4 files, 12 obsolete files)

[Final] Bug 1114515 - Patch1: Revise BluetoothGattHALInterface for discovering all services and first characteristic/descriptor. r=btian
7.22 KB, patch
Details | Diff | Splinter Review
[Final] Bug 1114515 - Patch2: Add BluetoothUuid, BluetoothGattId[], BluetoothGattServiceId[] into bluetooth ipc protocol. r=btian
4.54 KB, patch
Details | Diff | Splinter Review
[Final] Bug 1114515 - Patch3: Implement GATT client DiscoverServices API. r=btian, r=mrbkap
33.83 KB, patch
Details | Diff | Splinter Review
[Final] Bug 1114515 - Patch4: Implement Service, Characteristic, Descriptor interfaces for GATT client DiscoverServices API. r=btian, r=mrbkap
31.58 KB, patch
Details | Diff | Splinter Review
In the wiki doc[1], connectGatt() should also cover: Discovers services offered by a remote LE device as well as their characteristics and descriptors, and then. This bug is for implementing this part. [1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothDevice#connectGatt.28boolean_aAutoConnect.29
Based on BT team discussion, we decided to expose a service discovery API under BluetoothGatt instead of doing this operation in connect API.
Summary: Retrieve services, characteristics, descriptors from remote gatt server while connectGatt() → [Bluetooth2] Implement GATT service discovery API
This bug will cover 1) Basic implementation(constructor, attributes) of BluetoothGattService[1], BluetoothGattCharacteristic[2], BluetoothGattDescriptor[3] 2) discoverServices API under BluetoothGatt [4] [1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGattService [2] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGattCharacteristic [3] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGattDescriptor [4] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGatt#discoverServices.28.29 methods in BluetoothGattCharacteristic and BluetoothGattDescriptor will be covered by Bug 1082999.
Blocks: 1082999
No longer depends on: 1082999
Assignee: nobody → joliu
Hi Ben, I have a wrong commit message for patch 1, it should be "Revise BluetoothGattHALInterface for discovering all services and first characteristic/descriptor." I will update it in next revision or before landing, sorry for inconvenience. Thanks, Jocelyn
Blocks: 1140952
Blocks: 1140951
Comment on attachment 8573804 [details] [diff] [review] Bug 1114515 - Patch2: Add BluetoothUuid, BluetoothGattId[], BluetoothGattServiceId[] into bluetooth ipc protocol. Review of attachment 8573804 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nit addressed. ::: dom/bluetooth2/ipc/BluetoothTypes.ipdlh @@ +8,5 @@ > + from "mozilla/dom/bluetooth/BluetoothCommon.h"; > +using mozilla::dom::bluetooth::BluetoothGattId > + from "mozilla/dom/bluetooth/BluetoothCommon.h"; > +using mozilla::dom::bluetooth::BluetoothGattServiceId > + from "mozilla/dom/bluetooth/BluetoothCommon.h"; nit: sort by alphabetical order of type name.
Attachment #8573804 - Flags: review?(btian) → review+
Comment on attachment 8573803 [details] [diff] [review] Bug 1114515 - Patch1: Revise BluetoothGattHALInterface for discovering the first service/characteristic/descriptor. Review of attachment 8573803 [details] [diff] [review]: ----------------------------------------------------------------- Please see my comment below. ::: dom/bluetooth2/BluetoothInterface.h @@ +690,5 @@ > BluetoothGattClientResultHandler* aRes) = 0; > > /* Enumerate Attributes */ > virtual void SearchService(int aConnId, > + const BluetoothUuid* aUuid, How about add a boolean argument |aSearchAll| / |aStartFromFirst|? It's more explicit to indicate different cases. Use nullity of pointer is implicit and lack of argument type consistency.
Attachment #8573803 - Flags: review?(btian)
* Use boolean value for search all and first char/desc cases.
Attachment #8573803 - Attachment is obsolete: true
Attachment #8574721 - Flags: review?(btian)
* Revise based on Patch1(v2).
Attachment #8573805 - Attachment is obsolete: true
Attachment #8573805 - Flags: review?(btian)
Attachment #8574723 - Flags: review?(btian)
Comment on attachment 8574721 [details] [diff] [review] Bug 1114515 - Patch1(v2): Revise BluetoothGattHALInterface for discovering all services and first characteristic/descriptor. Review of attachment 8574721 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comment addressed. ::: dom/bluetooth2/BluetoothInterface.h @@ +691,5 @@ > > /* Enumerate Attributes */ > virtual void SearchService(int aConnId, > const BluetoothUuid& aUuid, > + bool aSearchAll, nit: move these boolean arguments before BluetoothUuid / BluetoothGattId to conform with logical order: 1) check SearchAll/First or not and then 2) if not, look the optional argument. ::: dom/bluetooth2/bluedroid/BluetoothGattHALInterface.cpp @@ +656,5 @@ > btgatt_srvc_id_t startServiceId; > > + if (aFirst && NS_SUCCEEDED(Convert(aServiceId, serviceId))) { > + status = mInterface->get_included_service(aConnId, &serviceId, 0); > + } else if (NS_SUCCEEDED(Convert(aServiceId, serviceId)) && Convert only |startServiceId| here since |serviceId| is converted in prior if condition. @@ +687,5 @@ > btgatt_gatt_id_t startCharId; > > + if (aFirst && NS_SUCCEEDED(Convert(aServiceId, serviceId))) { > + status = mInterface->get_characteristic(aConnId, &serviceId, 0); > + } else if (NS_SUCCEEDED(Convert(aServiceId, serviceId)) && Ditto. @@ +721,5 @@ > + NS_SUCCEEDED(Convert(aServiceId, serviceId)) && > + NS_SUCCEEDED(Convert(aCharId, charId))) { > + status = mInterface->get_descriptor(aConnId, &serviceId, &charId, 0); > + } else if (NS_SUCCEEDED(Convert(aServiceId, serviceId)) && > + NS_SUCCEEDED(Convert(aCharId, charId)) && Ditto.
Attachment #8574721 - Flags: review?(btian) → review+
Comment on attachment 8574723 [details] [diff] [review] Bug 1114515 - Patch3(v2): Implement GATT client DiscoverServices API. Review of attachment 8574723 [details] [diff] [review]: ----------------------------------------------------------------- Please see my comment below. ::: dom/bluetooth2/BluetoothGatt.h @@ +113,5 @@ > + > + /** > + * Indicate whether there is ongoing discoverServices request or not. > + */ > + bool mIsDiscovering; Is variable name |mDiscovering| better? We use |mDiscovering| in BluetoothAdapter: more consistent but confusing as well. ::: dom/bluetooth2/BluetoothUtils.cpp @@ +43,5 @@ > void > +ReversedUuidToString(const BluetoothUuid& aUuid, nsAString& aString) > +{ > + BluetoothUuid uuid; > + for (uint8_t i = 0; i< 16; i++) { nit: space before < @@ +44,5 @@ > +ReversedUuidToString(const BluetoothUuid& aUuid, nsAString& aString) > +{ > + BluetoothUuid uuid; > + for (uint8_t i = 0; i< 16; i++) { > + uuid.mUuid[15-i] = aUuid.mUuid[i]; Prefer to fill the array from head, but I'm fine in either way. uuid.mUuid[i] = aUuid.mUuid[15 - i]; ::: dom/bluetooth2/BluetoothUtils.h @@ +40,5 @@ > + * string representation. > + */ > +void > +ReversedUuidToString(const BluetoothUuid& aUuid, nsAString& aString); > +/** nit: newline here. ::: dom/bluetooth2/bluedroid/BluetoothGattManager.cpp @@ +73,5 @@ > + BluetoothSignal signal( > + NS_LITERAL_STRING("DiscoverCompleted"), > + mAppUuid, > + BluetoothValue(aSuccess)); > + bs->DistributeSignal(signal); Simplify here and all the following, as result of bug 1137103. bs->DistributeSignal( NS_LITERAL_STRING("DiscoverCompleted"), mAppUuid, BluetoothValue(aSuccess)); @@ +560,5 @@ > + size_t index = sClients->IndexOf(aAppUuid, 0 /* Start */, UuidComparator()); > + MOZ_ASSERT(index != sClients->NoIndex); > + > + nsRefPtr<BluetoothGattClient> client = sClients->ElementAt(index); > + MOZ_ASSERT(client->mConnId); Should mConnId > 0? @@ +761,5 @@ > + NS_ENSURE_TRUE_VOID(bs); > + > + size_t index = sClients->IndexOf(aConnId, 0 /* Start */, > + ConnIdComparator()); > + NS_ENSURE_TRUE_VOID(index != sClients->NoIndex); Should here be MOZ_ASSERT? Some of your code uses NS_ENSURE and others MOZ_ASSERT. Why is the difference? @@ +1041,5 @@ > +BluetoothGattManager::GeneratePathFromGattId(const BluetoothGattId& aId, > + nsAString& aPath) > +{ > + ReversedUuidToString(aId.mUuid, aPath); > + aPath.AppendInt(aId.mInstanceId); Insert a special character (e.g., '_') between uuid and instance id, in case we want to print it out for debugging. @@ +1047,5 @@ > + > +void > +BluetoothGattManager::ProceedDiscoverProcess( > + int aIndex, > + int aConnId, Replace |aIndex| and |aConnId| with BluetoothClient* directly since |aConnId| equals to |client->mConnId|. @@ +1059,5 @@ > + * There are three cases here, > + * 1) mCharacteristics is not empty: > + * Proceed to discover descriptors of the first saved characteristic. > + * 2) mCharacteristics is empty but mServices is not empty: > + * This service don't have any saved characteristics left, proceed to nit: doesn't ::: dom/bluetooth2/bluedroid/BluetoothGattManager.h @@ +141,5 @@ > + void GeneratePathFromGattId(const BluetoothGattId& aId, nsAString& aPath); > + > + void ProceedDiscoverProcess(int aIndex, > + int aConnId, > + const BluetoothGattServiceId& aServiceId); nit: move this section before |mInShutDown| to group function declaration.
Attachment #8574723 - Flags: review?(btian)
Jocelyn, please ignore my comment below and keep these conversion. The conversion of initial if-condition may not execute if |aSearchAll| / |aFirst| is false. (In reply to Ben Tian [:btian] from comment #12) > ::: dom/bluetooth2/bluedroid/BluetoothGattHALInterface.cpp > @@ +656,5 @@ > > btgatt_srvc_id_t startServiceId; > > > > + if (aFirst && NS_SUCCEEDED(Convert(aServiceId, serviceId))) { > > + status = mInterface->get_included_service(aConnId, &serviceId, 0); > > + } else if (NS_SUCCEEDED(Convert(aServiceId, serviceId)) && > > Convert only |startServiceId| here since |serviceId| is converted in prior > if condition. > > @@ +687,5 @@ > > btgatt_gatt_id_t startCharId; > > > > + if (aFirst && NS_SUCCEEDED(Convert(aServiceId, serviceId))) { > > + status = mInterface->get_characteristic(aConnId, &serviceId, 0); > > + } else if (NS_SUCCEEDED(Convert(aServiceId, serviceId)) && > > Ditto. > > @@ +721,5 @@ > > + NS_SUCCEEDED(Convert(aServiceId, serviceId)) && > > + NS_SUCCEEDED(Convert(aCharId, charId))) { > > + status = mInterface->get_descriptor(aConnId, &serviceId, &charId, 0); > > + } else if (NS_SUCCEEDED(Convert(aServiceId, serviceId)) && > > + NS_SUCCEEDED(Convert(aCharId, charId)) && > > Ditto.
Comment on attachment 8573807 [details] [diff] [review] Bug 1114515 - Patch4: Implement Service, Characteristic, Descriptor interfaces for GATT client DiscoverServices API. Review of attachment 8573807 [details] [diff] [review]: ----------------------------------------------------------------- Please see my comment below. ::: dom/bluetooth2/BluetoothGatt.cpp @@ +221,5 @@ > + > + for (uint32_t i = 0; i < serviceIds.Length(); i++) { > + nsRefPtr<BluetoothGattService> service = > + new BluetoothGattService(GetOwner(), mAppUuid, serviceIds[i]); > + mServices.AppendElement(service); How about simplify as following since |includedService| is redundant? I'm also fine to keep it. mServices.AppendElement( new BluetoothGattService(GetParentObject(), mAppUuid, serviceIds[i])); ::: dom/bluetooth2/BluetoothGattCharacteristic.cpp @@ +45,5 @@ > + > + BluetoothService* bs = BluetoothService::Get(); > + NS_ENSURE_TRUE_VOID(bs); > + > + // Generate a string representation to provide uuid of this service to nit: this 'characteristic' @@ +50,5 @@ > + // applications > + ReversedUuidToString(aCharId.mUuid, mUuid); > + > + nsString path = mUuid; > + path.AppendInt(mCharId.mInstanceId); Can we make |BluetoothGattManager::GeneratePathFromGattId| utility function? We can have one optional argument to output intermediate string (UUID only string). @@ +77,5 @@ > + for (uint32_t i = 0; i < descriptorIds.Length(); i++) { > + nsRefPtr<BluetoothGattDescriptor> descriptor = > + new BluetoothGattDescriptor( > + GetParentObject(), mAppUuid, mServiceId, > + this, mCharId, descriptorIds[i]); |mDescriptors.AppendElement| here. We can simplify as following: mDescriptors.AppendElement(new BluetoothGattDescriptor(...)); for here and BluetoothGattService. ::: dom/bluetooth2/BluetoothGattCharacteristic.h @@ +44,5 @@ > + } > + > + void GetUuid(nsString& aUuid) const > + { > + aUuid = mUuid; Rename to |mUuidStr| and |aUuidStr| to avoid ambiguity. @@ +101,5 @@ > + > + /** > + * ServiceId of the GATT service that this characteristic belongs to. > + */ > + BluetoothGattServiceId mServiceId; How about having a |BluetoothGattService::ServiceId| function to query service id from |mService| directly? @@ +110,5 @@ > + nsTArray<nsRefPtr<BluetoothGattDescriptor>> mDescriptors; > + > + /** > + * GattId of this GATT characteristic which contains > + * 1) mUuid: Uuid of this characteristic in byte array format. nit: UUID @@ +118,5 @@ > + > + /** > + * UUID string of this GATT characteristic. > + */ > + nsString mUuid; Rename to |mUuidStr| to be clear. Here, BluetoothGattDescriptor, and BluetoothGattService. ::: dom/bluetooth2/BluetoothGattDescriptor.cpp @@ +43,5 @@ > + MOZ_ASSERT(aOwner); > + MOZ_ASSERT(!mAppUuid.IsEmpty()); > + MOZ_ASSERT(aCharacteristic); > + > + // Generate a string representation to provide uuid of this service to nit: replace 'service' with 'descriptor' ::: dom/bluetooth2/BluetoothGattDescriptor.h @@ +75,5 @@ > + > + /** > + * ServiceId of the GATT service that this descriptor belongs to. > + */ > + BluetoothGattServiceId mServiceId; What is it for? @@ +85,5 @@ > + > + /** > + * GattId of the GATT characteristic that this descriptor belongs to. > + */ > + BluetoothGattId mCharId; What is it for? @@ +89,5 @@ > + BluetoothGattId mCharId; > + > + /** > + * GattId of this GATT descriptor which contains > + * 1) mUuid: Uuid of this descriptor in byte array format. nit: UUID ::: dom/bluetooth2/BluetoothGattService.h @@ +108,5 @@ > + nsString mAppUuid; > + > + /** > + * ServiceId of this GATT service which contains > + * 1) mId.mUuid: Uuid of this service in byte array format. nit: "UUID of ..." to be consistent.
Attachment #8573807 - Flags: review?(btian)
(In reply to Ben Tian [:btian] from comment #15) > Comment on attachment 8573807 [details] [diff] [review] > Bug 1114515 - Patch4: Implement Service, Characteristic, Descriptor > interfaces for GATT client DiscoverServices API. > > Review of attachment 8573807 [details] [diff] [review]: > ----------------------------------------------------------------- > > Please see my comment below. > > @@ +101,5 @@ > > + > > + /** > > + * ServiceId of the GATT service that this characteristic belongs to. > > + */ > > + BluetoothGattServiceId mServiceId; > > How about having a |BluetoothGattService::ServiceId| function to query > service id from |mService| directly? > > ::: dom/bluetooth2/BluetoothGattDescriptor.h > @@ +75,5 @@ > > + > > + /** > > + * ServiceId of the GATT service that this descriptor belongs to. > > + */ > > + BluetoothGattServiceId mServiceId; > > What is it for? > > @@ +85,5 @@ > > + > > + /** > > + * GattId of the GATT characteristic that this descriptor belongs to. > > + */ > > + BluetoothGattId mCharId; > > What is it for? > These two are used when we want to read/write descriptor, we will need service, char, and desc id. Will you recommend we add |mCharacteristic| and query charId and servId from mCharacteristic and mCharacteristic.mService every time?
(In reply to jocelyn [:jocelyn] from comment #16) > These two are used when we want to read/write descriptor, we will need > service, char, and desc id. I see. > Will you recommend we add |mCharacteristic| and query charId and servId from > mCharacteristic and mCharacteristic.mService every time? Yes I'm thinking a lightweight getter since |mService| and |mServiceId| seem a little duplicate. Let me know your thought.
Flags: needinfo?(joliu)
Comment on attachment 8574723 [details] [diff] [review] Bug 1114515 - Patch3(v2): Implement GATT client DiscoverServices API. Review of attachment 8574723 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth2/bluedroid/BluetoothGattManager.cpp @@ +761,5 @@ > + NS_ENSURE_TRUE_VOID(bs); > + > + size_t index = sClients->IndexOf(aConnId, 0 /* Start */, > + ConnIdComparator()); > + NS_ENSURE_TRUE_VOID(index != sClients->NoIndex); Yes, should be MOZ_ASSERT.
(In reply to Ben Tian [:btian] from comment #17) > (In reply to jocelyn [:jocelyn] from comment #16) > > These two are used when we want to read/write descriptor, we will need > > service, char, and desc id. > > I see. > > > Will you recommend we add |mCharacteristic| and query charId and servId from > > mCharacteristic and mCharacteristic.mService every time? > > Yes I'm thinking a lightweight getter since |mService| and |mServiceId| seem > a little duplicate. Let me know your thought. Agreed. How about adding |const BluetoothGattServiceId& GetServiceId() const;| in |BluetoothGattService|, and a similar thing in |BluetoothGattCharacteristic|?
Flags: needinfo?(joliu)
(In reply to jocelyn [:jocelyn] from comment #19) > Agreed. > How about adding |const BluetoothGattServiceId& GetServiceId() const;| in > |BluetoothGattService|, and a similar thing in |BluetoothGattCharacteristic|? Sure. Please go ahead.
- address review comments - move GeneratePathFromGattId to BluetoothUtils for future usage.
Attachment #8574723 - Attachment is obsolete: true
Attachment #8578391 - Flags: review?(btian)
- address review comments - use |GeneratePathFromGattId| utility when register/unregister signals - remove unnecessary copy for AppUuid, ServiceId, CharacteristicId, add getter in |BluetoothGattService| and |BluetoothGattCharacteristic| instead.
Attachment #8573807 - Attachment is obsolete: true
Attachment #8578393 - Flags: review?(btian)
Comment on attachment 8578391 [details] [diff] [review] Bug 1114515 - Patch3(v3): Implement GATT client DiscoverServices API. Review of attachment 8578391 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comment addressed. ::: dom/bluetooth2/BluetoothGatt.h @@ +113,5 @@ > + > + /** > + * Indicate whether there is ongoing discoverServices request or not. > + */ > + bool mDiscovering; Rename to |mDiscoveringServices| to be clearer. ::: dom/bluetooth2/BluetoothUtils.cpp @@ +77,5 @@ > > +void > +GeneratePathFromGattId(const BluetoothGattId& aId, > + nsAString& aUuidStr, > + nsAString& aPath) 2nd argument should be |aPath|. ::: dom/bluetooth2/BluetoothUtils.h @@ +70,5 @@ > +/** > + * Generate bluetooth signal path from a GattId. > + * > + * @param aId [in] GattId value to convert. > + * @param aPath [out] Register signal path generated from aId. nit: 'Bluetooth' signal path as another |GeneratePathFromGattId|'s comment.
Attachment #8578391 - Flags: review?(btian) → review+
Comment on attachment 8578393 [details] [diff] [review] Bug 1114515 - Patch4(v2): Implement Service, Characteristic, Descriptor interfaces for GATT client DiscoverServices API. Review of attachment 8578393 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. Please request DOM peer review for webidl change.
Attachment #8578393 - Flags: review?(btian) → review+
- address review comments and rebase master Hi Blake, This bug is for implementing DiscoverServices[1] API in our GATT client API. This method is to discover all GattService[2], GattCharacteristic[3], GattDescriptor[4] provided by the remote BLE server. May I ask for your help to review on patch 3 and patch 4 from DOM peer point of view? Patch summary: Patch3: ** 1) BluetoothGatt.webidl/cpp/h: Add discoverServices method in BluetoothGatt interface and implement it. 2) BluetoothGattManager.cpp/h: Interact with bluetooth stack and distribute signals to our content process. 3) Other files: adding IPC requests Patch4: Add and implement basic properties in BluetoothGattService/Characteristic/Descriptor [1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGatt#discoverServices.28.29 [2] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGattService#BluetoothGattService [3] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGattCharacteristic#BluetoothGattCharacteristic [4] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGattDescriptor#BluetoothGattDescriptor Thanks, Jocelyn
Attachment #8578391 - Attachment is obsolete: true
Attachment #8579951 - Flags: review?(mrbkap)
Attachment #8579951 - Flags: review?(mrbkap) → review+
Comment on attachment 8579954 [details] [diff] [review] Bug 1114515 - Patch4(v2): Implement Service, Characteristic, Descriptor interfaces for GATT client DiscoverServices API. r=btian Review of attachment 8579954 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth2/BluetoothGattService.cpp @@ +62,5 @@ > +{ > + MOZ_ASSERT(aValue.type() == BluetoothValue::TArrayOfBluetoothGattServiceId); > + > + const InfallibleTArray<BluetoothGattServiceId>& includedServIds > + = aValue.get_ArrayOfBluetoothGattServiceId(); Nit: Please leave the '=' on the previous line.
Attachment #8579954 - Flags: review?(mrbkap) → review+
Thanks for reviewing, Ben and Blake! No try server result since bluetooth2 won't be built.
Keywords: checkin-needed
Removed the keyword since it needs to rebase after Bug 1117172 and Bug 1145631 landed.
Keywords: checkin-needed
Sorry, wrong bug referenced in the Comment 35. Should be Bug 1117172 and Bug 1145631.
No try server link since bluetooth2 won't be built.
Keywords: checkin-needed
Blocks: 1146293
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: