All users were logged out of Bugzilla on October 13th, 2018

[bluetooth2] Characteristics and descriptors arrays are empty in first few connections

RESOLVED FIXED in Firefox 41

Status

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: elin, Assigned: brsun)

Tracking

unspecified
2.2 S14 (12june)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

(Whiteboard: [webbt-api])

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 8601976 [details]
empty-descriptors.log

I've created a Ble Explorer App:
https://github.com/elin-moco/ble-explorer/blob/master/main.js

and found that when I first connect to my LE device(Arduino+BleShield),
the service.characteristics array is empty,
if I disconnect and connect again, characteristics appears,
but the characteristic.descriptors array is still empty,
it appears after I repeat the disconnect/connect action again.

Not sure if the last few logs would help clarify the situation.
(Reporter)

Comment 1

4 years ago
Another note to this bug,
this happens only on first device connected after app starts,
if I repeat connect/disconnect until I can access descriptors for one device,
when I switch to another device, I won't need to repeat connect/disconnect again.
Whiteboard: [webbt-api]
Assignee: nobody → brsun
I'll take a first look, thanks for reporting. ;)
Assignee: brsun → joliu
Hi Eddie,

Other than reconnect the device, what's the result of calling discoverServices again?

Updated

4 years ago
Blocks: 1165848
(Reporter)

Comment 4

4 years ago
Hi Jocelyn:


I tried to call discoverServices again, and it worked!
Seems like I only need to call it a few more times!
(In reply to elin from comment #4)
> Hi Jocelyn:
> 
> 
> I tried to call discoverServices again, and it worked!
> Seems like I only need to call it a few more times!

Hi Eddie,

We will investigate the reason that why the lists are empty sometimes.
Please do discoverServices again to get the latest list before we fix this bug.

Thanks,
Jocelyn
(Assignee)

Comment 6

3 years ago
This is a timing issue.

[Discovery procedures in the parent process]
(a) Discover services (triggered by the child process).
(b) Notify the corresponding BluetoothGatt object in the child process about "ServicesDiscovered".
(c) Notify the corresponding BluetoothService object in the child process about "IncludedServicesDiscovered".
(d) Notify the corresponding BluetoothGattService object in the child process about "CharacteristicsDiscovered".
(e) Notify the corresponding BluetoothGattCharacteristic object in the child process about "DescriptorsDiscovered".

[Discovery procedures in the child process]
(a) One BluetoothGatt object is created and registered to receive notification from the parent process.
(b) One BluetoothGatt object notifies the parent process to start discovery procedures.
(c) After one BluetoothGatt object receives "ServicesDiscovered", several BluetoothGattService objects are created and registered to receive notification from the parent process.
(d) After one BluetoothGattService object receives "CharacteristicsDiscovered", several BluetoothGattCharacteristic objects are created and registered to receive notification from the parent process.
(e) After one BluetoothGattCharacteristic object receives "DescriptorsDiscovered", several BluetoothGattDescriptor objects are created and registered to receive notification from the parent process.

Since the IPC protocols used in PBluetooth are asynchronous, there are no guarantees that corresponding objects in the child process can be created and registered before the parent process sends the notification. If "CharacteristicsDiscovered" and "DescriptorsDiscovered" notifications are sent earlier than the creation (and registration) of BluetoothGattService and BluetoothGattCharacteristic, the later created (or registered) objects won't be able to update its corresponding array for characteristics and descriptors.

Comment 7

3 years ago
FYR. One possible fix is, as bug 1100818, to queue these events until object is registered. But it further messes BluetoothService code.
(Assignee)

Comment 8

3 years ago
Created attachment 8610076 [details] [diff] [review]
bug1161945_gatt_client_empty_characteristic_descriptor.patch

Hi Eddie,

Would you please help try this patch to see if it helps on this issue or not?
Attachment #8610076 - Flags: feedback?(elin)
(Reporter)

Comment 9

3 years ago
OK, I could try it tomorrow.(In reply to Bruce Sun [:brsun] from comment #8)
> Created attachment 8610076 [details] [diff] [review]
> bug1161945_gatt_client_empty_characteristic_descriptor.patch
> 
> Hi Eddie,
> 
> Would you please help try this patch to see if it helps on this issue or not?

Cool, I'll try it tomorrow.
Assignee: joliu → brsun
(Reporter)

Comment 10

3 years ago
Comment on attachment 8610076 [details] [diff] [review]
bug1161945_gatt_client_empty_characteristic_descriptor.patch

I've tried it on my flame and it works!
Thanks for the work!
Attachment #8610076 - Flags: feedback?(elin) → feedback+
(Assignee)

Updated

3 years ago
Attachment #8610076 - Flags: review?(joliu)
Comment on attachment 8610076 [details] [diff] [review]
bug1161945_gatt_client_empty_characteristic_descriptor.patch

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

Hi Bruce,

Please see my comments below.

Thanks,
Jocelyn

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +1631,5 @@
>        new DiscoverResultHandler(client));
>    } else { // all characteristics of this service are discovered
> +    // Notify BluetoothGatt to make BluetoothGattService create characteristics
> +    // then proceed
> +    nsTArray<BluetoothNamedValue> ids;

nit: but characteristics below is not an id array?

::: dom/bluetooth/bluetooth2/BluetoothGatt.cpp
@@ +287,5 @@
> +  MOZ_ASSERT(aValue.type() == BluetoothValue::TArrayOfBluetoothNamedValue);
> +
> +  const InfallibleTArray<BluetoothNamedValue>& ids =
> +    aValue.get_ArrayOfBluetoothNamedValue();
> +  MOZ_ASSERT(ids.Length() == 2); // ServiceId, ServiceIds

s/ServiceIds/IncludedServiceIds

@@ +309,5 @@
> +  MOZ_ASSERT(aValue.type() == BluetoothValue::TArrayOfBluetoothNamedValue);
> +
> +  const InfallibleTArray<BluetoothNamedValue>& ids =
> +    aValue.get_ArrayOfBluetoothNamedValue();
> +  MOZ_ASSERT(ids.Length() == 2); // ServiceId, CharIds

nit: GattCharAttribute contains more than characteristic ids, no?

::: dom/bluetooth/bluetooth2/BluetoothGattCharacteristic.h
@@ -106,5 @@
>     * Add newly discovered GATT descriptors into mDescriptors and update the
>     * cache value of mDescriptors.
> -   *
> -   * @param aValue [in] BluetoothValue which contains an array of
> -   *                    BluetoothGattId of all discovered descriptors.

Please see my comments in BluetoothGattService.h.

::: dom/bluetooth/bluetooth2/BluetoothGattService.cpp
@@ +121,5 @@
>    BT_LOGD("[D] %s", NS_ConvertUTF16toUTF8(aData.name()).get());
>    NS_ENSURE_TRUE_VOID(mSignalRegistered);
>  
>    BluetoothValue v = aData.value();
> +  if (true) {

Wouldn't it be better that we don't register the signal handler in the first place if there're no signals need to be handled?

::: dom/bluetooth/bluetooth2/BluetoothGattService.h
@@ -93,5 @@
>     * update the cache value of mIncludedServices.
> -   *
> -   * @param aValue [in] BluetoothValue which contains an array of
> -   *                    BluetoothGattServiceId of all discovered included
> -   *                    services.

nit: Though it's not hard to understand, I would suggest to still have a param documentation here and all similar functions here to be consistent. But I'm OK in either way.
Attachment #8610076 - Flags: review?(joliu)
(Assignee)

Comment 12

3 years ago
Created attachment 8613472 [details] [diff] [review]
bug1161945_gatt_client_empty_characteristic_descriptor.v2.patch

Address suggestions on comment 11.

I also move the reverse logic of UUID from BluetoothUtils into BluetoothHALHelpers because the reverse stuff seems not related to normal/general use cases outside BluetoothInterface.
Attachment #8610076 - Attachment is obsolete: true
Attachment #8613472 - Flags: review?(joliu)
Comment on attachment 8613472 [details] [diff] [review]
bug1161945_gatt_client_empty_characteristic_descriptor.v2.patch

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

Hi Bruce,

Totally agree that we should move the reversed logic, but please also revise bluetooth daemon.
Otherwise, applications will see the reversed UUID when using daemon. (which should be the default backend.)

Thanks,
Jocelyn
Attachment #8613472 - Flags: review?(joliu)
(Assignee)

Comment 14

3 years ago
Got it. This patch just didn't have a good timing to be committed. I'll complete the daemon part as well.
(Assignee)

Updated

3 years ago
Depends on: 1171866
(Assignee)

Updated

3 years ago
Depends on: 1171868
(Assignee)

Comment 15

3 years ago
Created attachment 8616529 [details] [diff] [review]
bug1161945_gatt_client_empty_characteristic_descriptor.v3.patch
Attachment #8613472 - Attachment is obsolete: true
Attachment #8616529 - Flags: review?(joliu)
Comment on attachment 8616529 [details] [diff] [review]
bug1161945_gatt_client_empty_characteristic_descriptor.v3.patch

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

r=me after comments addressed, thanks.

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +1643,5 @@
>        new DiscoverResultHandler(client));
>    } else { // all characteristics of this service are discovered
> +    // Notify BluetoothGatt to make BluetoothGattService create characteristics
> +    // then proceed
> +    nsTArray<BluetoothNamedValue> value;

nit: values

@@ +1645,5 @@
> +    // Notify BluetoothGatt to make BluetoothGattService create characteristics
> +    // then proceed
> +    nsTArray<BluetoothNamedValue> value;
> +    value.AppendElement(
> +      BluetoothNamedValue(NS_LITERAL_STRING("serviceId"), aServiceId));

Suggest to use
BT_APPEND_NAMED_VALUE(values, "serviceId", aServiceId);
for here and all similar cases in this patch.

@@ +1692,5 @@
>        new DiscoverResultHandler(client));
>    } else { // all descriptors of this characteristic are discovered
> +    // Notify BluetoothGatt to make BluetoothGattCharacteristic create
> +    // descriptors then proceed
> +    nsTArray<BluetoothNamedValue> ids;

How about use values here and below?

::: dom/bluetooth/bluetooth2/BluetoothGatt.cpp
@@ +285,5 @@
> +BluetoothGatt::HandleIncludedServicesDiscovered(const BluetoothValue& aValue)
> +{
> +  MOZ_ASSERT(aValue.type() == BluetoothValue::TArrayOfBluetoothNamedValue);
> +
> +  const InfallibleTArray<BluetoothNamedValue>& ids =

I will suggest to use a more general variable name such as 'values' instead of 'ids' for the arrays in these Handle* functions.
Sometimes it's a bit of confusing, for example, in HandleGattCharactersiticDiscovered, this array is not just ids, but it is indeed a value array.
Since we could easily see their value types in the function, I think we could just use 'values' for the array name.

@@ +287,5 @@
> +  MOZ_ASSERT(aValue.type() == BluetoothValue::TArrayOfBluetoothNamedValue);
> +
> +  const InfallibleTArray<BluetoothNamedValue>& ids =
> +    aValue.get_ArrayOfBluetoothNamedValue();
> +  MOZ_ASSERT(ids.Length() == 2); // ServiceId, IncludedServiceIds

Sorry, let's keep it IncludedServices to be consistent with its name and use values for the array name.

@@ +331,5 @@
> +  MOZ_ASSERT(aValue.type() == BluetoothValue::TArrayOfBluetoothNamedValue);
> +
> +  const InfallibleTArray<BluetoothNamedValue>& ids =
> +    aValue.get_ArrayOfBluetoothNamedValue();
> +  MOZ_ASSERT(ids.Length() == 3); // ServiceId, CharId, DescriptorIds

Suggest to use values as the array name, and s/DescriptorIds/Descriptors here.

::: dom/bluetooth/bluetooth2/BluetoothGatt.h
@@ +134,5 @@
> +   *                    name and uses BluetoothGattServiceId as the value. The
> +   *                    second element uses 'characteristicId' as the name and
> +   *                    uses BluetoothGattId as the value. The third element
> +   *                    uses 'descriptors' as an array of BluetoothGattId of all
> +   *                    discovered descriptors as the value.

The third element uses 'descriptors' as the name and uses an array...

::: dom/bluetooth/bluetooth2/BluetoothGattService.cpp
@@ -33,5 @@
> -   * after unlinked. Please see Bug 1138267 for detail informations.
> -   */
> -  nsString path;
> -  GeneratePathFromGattId(tmp->mServiceId.mId, path);
> -  UnregisterBluetoothSignalHandler(path, tmp);

Since we don't need to implement the unlink function by our self, we could use NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(BluetoothGattService, mOwner, mIncludedServices, mCharacteristics) directly, and remove lines from NS_IMPL_CYCLE_COLLECTION_CLASS(BluetoothGattService) to NS_IMPL_CYCLE_COLLECTION_TRACE_WRAPPERCACHE(BluetoothGattService).
Attachment #8616529 - Flags: review?(joliu) → review+
(Assignee)

Comment 17

3 years ago
Created attachment 8620889 [details] [diff] [review]
bug1161945_gatt_client_empty_characteristic_descriptor.commit.patch

Address comment 16.
Attachment #8616529 - Attachment is obsolete: true
Attachment #8620889 - Flags: review+
(Assignee)

Comment 18

3 years ago
GATT related features are used in Bluetooth API v2, which is not covered by try server currently.
Keywords: checkin-needed
(Assignee)

Comment 19

3 years ago
Created attachment 8620907 [details] [diff] [review]
bug1161945_gatt_client_empty_characteristic_descriptor.commit.patch

Add f=elin
Attachment #8620889 - Attachment is obsolete: true
Attachment #8620907 - Flags: review+
Attachment #8620907 - Flags: feedback+
sorry this failed to apply:

renamed 1161945 -> bug1161945_gatt_client_empty_characteristic_descriptor.commit.patch
applying bug1161945_gatt_client_empty_characteristic_descriptor.commit.patch
patching file dom/bluetooth/bluetooth2/BluetoothGattService.cpp
Hunk #2 FAILED at 34
1 out of 2 hunks FAILED -- saving rejects to file dom/bluetooth/bluetooth2/BluetoothGattService.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh bug1161945_gatt_client_empty_characteristic_descriptor.commit.patch
can you take a look, thanks!
Flags: needinfo?(brsun)
Keywords: checkin-needed
(Assignee)

Comment 21

3 years ago
Created attachment 8621423 [details] [diff] [review]
bug1161945_gatt_client_empty_characteristic_descriptor.commit.v2.patch

Regenerate a patch based on the latest m-c (248462:c2a414f8bd73).
Attachment #8620907 - Attachment is obsolete: true
Flags: needinfo?(brsun)
Attachment #8621423 - Flags: review+
Attachment #8621423 - Flags: feedback+
(Assignee)

Comment 22

3 years ago
The attachment 8621423 [details] [diff] [review] basically is the same as the attachment 8620907 [details] [diff] [review]. Not sure about what causes the failure to apply the patch yet. Please kindly help share .rej files if this patch doesn't work again. Thanks in advance. :)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/727d31bebaa9
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S14 (12june)
You need to log in before you can comment on or make changes to this bug.