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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[elin]

Attachment: 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.

Comment 1

[elin]

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.

Comment 2

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

I'll take a first look, thanks for reporting. ;)

Comment 3

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

Hi Eddie,

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

Comment 4

[elin]

Hi Jocelyn:


I tried to call discoverServices again, and it worked!
Seems like I only need to call it a few more times!

Comment 5

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

(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

Comment 6

[Bruce Sun [:brsun]]

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

[Ben Tian (inactive)]

FYR. One possible fix is, as bug 1100818, to queue these events until object is registered. But it further messes BluetoothService code.

Comment 8

[Bruce Sun [:brsun]]

Attachment: 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?

Comment 9

[elin]

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.

Comment 10

[elin]

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!

Comment 11

[Jocelyn Liu [:jocelyn] [: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.

Comment 12

[Bruce Sun [:brsun]]

Attachment: 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.

Comment 13

[Jocelyn Liu [:jocelyn] [: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

Comment 14

[Bruce Sun [:brsun]]

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

Comment 15

[Bruce Sun [:brsun]]

Attachment: bug1161945_gatt_client_empty_characteristic_descriptor.v3.patch

Comment 16

[Jocelyn Liu [:jocelyn] [: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).

Comment 17

[Bruce Sun [:brsun]]

Attachment: bug1161945_gatt_client_empty_characteristic_descriptor.commit.patch

Address comment 16.

Comment 18

[Bruce Sun [:brsun]]

GATT related features are used in Bluetooth API v2, which is not covered by try server currently.

Comment 19

[Bruce Sun [:brsun]]

Attachment: bug1161945_gatt_client_empty_characteristic_descriptor.commit.patch

Add f=elin

Comment 20

[Carsten Book [:Tomcat]]

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!

Comment 21

[Bruce Sun [:brsun]]

Attachment: bug1161945_gatt_client_empty_characteristic_descriptor.commit.v2.patch

Regenerate a patch based on the latest m-c (248462:c2a414f8bd73).

Comment 22

[Bruce Sun [:brsun]]

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. :)

Comment 23

[Pulsebot]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/727d31bebaa9