[bluetooth2] Implement read/writeValue for GATT client API

RESOLVED FIXED in Firefox 40

Status

Firefox OS
Bluetooth
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jocelyn, Assigned: brsun)

Tracking

unspecified
2.2 S10 (17apr)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

(Whiteboard: [webbt-api])

Attachments

(4 attachments, 16 obsolete attachments)

8.29 KB, patch
brsun
: review+
Details | Diff | Splinter Review
60.83 KB, patch
brsun
: review+
Details | Diff | Splinter Review
4.29 KB, patch
brsun
: review+
Details | Diff | Splinter Review
44.62 KB, patch
brsun
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
This bug will cover
 - BluetoothGattCharacteristic.value 
 - BluetoothGattCharacteristic.read/WriteValue()
 - BluetoothGattDescriptor.value
 - BluetoothGattDescriptor.readWriteValue()

Hi Bruce,

Can you help on this bug?

Thanks,
Jocelyn
(Assignee)

Comment 1

3 years ago
Sure.
Assignee: nobody → brsun
(Assignee)

Comment 2

3 years ago
Created attachment 8579328 [details] [diff] [review]
bug1140952_gatt_read_write_value.wip.patch

WIP: read/write function of characteristic value.
(Assignee)

Comment 3

3 years ago
Created attachment 8579886 [details] [diff] [review]
bug1140952_gatt_read_write_value.prototype.patch

Implement the read/write function of characteristic/descriptor values based on patches from bug 1140952.

Hi Jocelyn,

May I have your feedback? :)
Attachment #8579328 - Attachment is obsolete: true
Attachment #8579886 - Flags: feedback?(joliu)
(Reporter)

Comment 4

3 years ago
Comment on attachment 8579886 [details] [diff] [review]
bug1140952_gatt_read_write_value.prototype.patch

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

Per offline discussion, please split this patch into several patches.

Thanks,
Jocelyn
Attachment #8579886 - Flags: feedback?(joliu)
(Assignee)

Comment 5

3 years ago
Based on the statement[1], we should queue all other requests before the response of the first request has received. Since there is not guarantee that the underlying bluetooth stack can queue requests for us, we should handle these sequential stuffs inside Gecko.

I am going to handle the sequential protocol in later patches.

[1] "Once a client sends a request to a server, that client shall send no other request to the same server until a response PDU has been received", from  Bluetooth Core Specification Vol.3, Part F, 3.3.2 Sequential Protocol.
(Assignee)

Comment 6

3 years ago
Per offline discussion, I will separate sequential stuffs mentioned on comment 5 to another bug.
(Assignee)

Comment 7

3 years ago
Created attachment 8583662 [details] [diff] [review]
bug1140952_01_gatt_read_write_characteristic_webapi.patch
Attachment #8579886 - Attachment is obsolete: true
Attachment #8583662 - Flags: feedback?(joliu)
(Assignee)

Comment 8

3 years ago
Created attachment 8583663 [details] [diff] [review]
bug1140952_02_gatt_read_write_characteristic_ipc.patch
Attachment #8583663 - Flags: feedback?(joliu)
(Assignee)

Comment 9

3 years ago
Created attachment 8583664 [details] [diff] [review]
bug1140952_03_gatt_read_write_descriptor_webapi.patch
Attachment #8583664 - Flags: feedback?(joliu)
(Assignee)

Comment 10

3 years ago
Created attachment 8583666 [details] [diff] [review]
bug1140952_04_gatt_read_write_descriptor_ipc.patch
Attachment #8583666 - Flags: feedback?(joliu)
(In reply to Bruce Sun [:brsun] from comment #5)
> Based on the statement[1], we should queue all other requests before the
> response of the first request has received. Since there is not guarantee
> that the underlying bluetooth stack can queue requests for us, we should
> handle these sequential stuffs inside Gecko.
> 
> I am going to handle the sequential protocol in later patches.
> 
> [1] "Once a client sends a request to a server, that client shall send no
> other request to the same server until a response PDU has been received",
> from  Bluetooth Core Specification Vol.3, Part F, 3.3.2 Sequential Protocol.

Good catch! I'm surprised we need to handle this in gecko.
(Assignee)

Comment 12

3 years ago
(In reply to Bruce Sun [:brsun] from comment #6)
> Per offline discussion, I will separate sequential stuffs mentioned on
> comment 5 to another bug.

Forget to mention, the corresponding follow-up bug is bug 1147776.
(Reporter)

Comment 13

3 years ago
Comment on attachment 8583662 [details] [diff] [review]
bug1140952_01_gatt_read_write_characteristic_webapi.patch

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

Hi Bruce,

Please see my comments below.

Thanks,
Jocelyn

::: dom/bluetooth2/BluetoothGattCharacteristic.cpp
@@ +34,5 @@
>    : mOwner(aOwner)
>    , mService(aService)
>    , mCharId(aCharId)
> +  , mProperties(BLUETOOTH_EMPTY_GATT_CHAR_PROP)
> +  , mWriteType(GATT_WRITE_TYPE_NO_RESPONSE)

Maybe we should remove DEFAULT keyword if the default write type is not GATT_WRITE_TYPE_DEFAULT?

::: dom/webidl/BluetoothGattCharacteristic.webidl
@@ +35,5 @@
> +  readonly attribute ArrayBuffer?                           value;
> +  [Cached, Constant]
> +  readonly attribute Properties 							properties;
> +  [Cached, Pure]
> +  readonly attribute WriteType                              writeType;

As we discussed offline, we don't need to expose writeType to application for GATT client API. We can do it when implementing server-side API.
Attachment #8583662 - Flags: feedback?(joliu)
(Reporter)

Comment 14

3 years ago
Comment on attachment 8583664 [details] [diff] [review]
bug1140952_03_gatt_read_write_descriptor_webapi.patch

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

LGTM.
Attachment #8583664 - Flags: feedback?(joliu) → feedback+
(Reporter)

Comment 15

3 years ago
Comment on attachment 8583663 [details] [diff] [review]
bug1140952_02_gatt_read_write_characteristic_ipc.patch

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

1) Shouldn't we update the characteristic value after writeCharacteristic operation?

2) Do we need to save each characteristic/descriptor info even there's no read/write request on it?
   Especially there is already one copy in the content process.
   Have you consider handle this retry in content process(additional ipc cost) or just save a copy for ongoing requests?

::: dom/bluetooth2/BluetoothCommon.h
@@ +618,5 @@
>      return mId == aOther.mId && mIsPrimary == aOther.mIsPrimary;
>    }
>  };
>  
> +struct BluetoothGattCharTrait {

IMO, 'Trait' does not fit the use case here.
Could you come up with another name to describe this?

::: dom/bluetooth2/BluetoothGattCharacteristic.cpp
@@ +95,5 @@
>    BluetoothValue v = aData.value();
>    if (aData.name().EqualsLiteral("DescriptorsDiscovered")) {
>      HandleDescriptorsDiscovered(v);
> +  } else if (aData.name().EqualsLiteral("CharacteristicValueUpdated")) {
> +    HandleCharacteristicValueUpdated(v);

nit: This function is quite trivial, how about we do it here directly?

@@ +168,5 @@
> +    NS_ENSURE_TRUE(v.type() == BluetoothValue::TArrayOfuint8_t, false);
> +
> +    AutoJSAPI jsapi;
> +    NS_ENSURE_TRUE(jsapi.Init(mCharacteristic->GetParentObject()), false);
> +    JSContext* cx = jsapi.cx();

nit: move the newline before this line.

@@ +225,5 @@
> +  static_assert(sizeof(*aValue.Data()) == sizeof(uint8_t),
> +                "byte-sized data required");
> +
> +  nsTArray<uint8_t> value;
> +  value.AppendElements<uint8_t>(aValue.Data(), aValue.Length());

Do we need to add <uint8_t> here?

::: dom/bluetooth2/bluedroid/BluetoothGattManager.cpp
@@ +41,5 @@
>  
>  static StaticAutoPtr<nsTArray<nsRefPtr<BluetoothGattClient> > > sClients;
>  
> +/**
> + * hashkey wrapper for BluetoothGattId

nit: Hashkey wrapper

@@ +735,5 @@
> +
> +  ENSURE_GATT_CLIENT_INTF_IS_READY_VOID(aRunnable);
> +
> +  size_t index = sClients->IndexOf(aAppUuid, 0 /* Start */, UuidComparator());
> +  if (NS_WARN_IF(index == sClients->NoIndex)) {

Will this case happen here?
ClientIf will only be unregistered in |BluetoothGatt::DisconnectFromOwner| and |BluetoothGatt::~BluetoothGatt|.
Shouldn't we use MOZ_ASSERT in these kind of things?

@@ +1087,5 @@
>      // discovering descriptors of this characteristic later
> +    client->mCharacteristics.AppendElement(pChar->mTrait);
> +
> +    client->mChars.Put(BluetoothGattIdHashKeyWrapper(aCharId),
> +                            pChar.forget());

nit: add a comment and fix the intent.
Attachment #8583663 - Flags: feedback?(joliu)
(Reporter)

Comment 16

3 years ago
Comment on attachment 8583666 [details] [diff] [review]
bug1140952_04_gatt_read_write_descriptor_ipc.patch

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

Two questions mentioned in the previous comment also applies for this patch.

::: dom/bluetooth2/BluetoothCommon.h
@@ +638,5 @@
> +  bool operator==(const BluetoothGattDescTrait& aOther) const
> +  {
> +    return mId == aOther.mId;
> +  }
> +};

Why don't we use BluetoothGattId directly?
More consistent with characteristic implementation?
Any other reasons?

::: dom/bluetooth2/BluetoothGattDescriptor.cpp
@@ +74,5 @@
> +  BT_LOGD("[D] %s", NS_ConvertUTF16toUTF8(aData.name()).get());
> +
> +  BluetoothValue v = aData.value();
> +  if (aData.name().EqualsLiteral("DescriptorValueUpdated")) {
> +    HandleDescriptorValueUpdated(v);

nit: This function is quite trivial, how about we do it here directly?

@@ +126,5 @@
> +    NS_ENSURE_TRUE(v.type() == BluetoothValue::TArrayOfuint8_t, false);
> +
> +    AutoJSAPI jsapi;
> +    NS_ENSURE_TRUE(jsapi.Init(mDescriptor->GetParentObject()), false);
> +    JSContext* cx = jsapi.cx();

nit: move the newline before this line.

@@ +154,5 @@
> +  NS_ENSURE_TRUE(!aRv.Failed(), nullptr);
> +
> +  BluetoothService* bs = BluetoothService::Get();
> +  BT_ENSURE_TRUE_REJECT(bs, NS_ERROR_NOT_AVAILABLE);
> +

Please check the characteristic property to see if this descriptor can be read.
We should reject the Promise directly if the read bit is not set.
Same thing applies to WriteValue() below and |BluetoothCharacteristic::Read/WriteValue()|

@@ +185,5 @@
> +  static_assert(sizeof(*aValue.Data()) == sizeof(uint8_t),
> +                "byte-sized data required");
> +
> +  nsTArray<uint8_t> value;
> +  value.AppendElements<uint8_t>(aValue.Data(), aValue.Length());

Do we need to add <uint8_t> here?

@@ +191,5 @@
> +  BluetoothService* bs = BluetoothService::Get();
> +  BT_ENSURE_TRUE_REJECT(bs, NS_ERROR_NOT_AVAILABLE);
> +
> +  nsRefPtr<BluetoothReplyRunnable> result = new BluetoothVoidReplyRunnable(
> +    nullptr, promise, NS_LITERAL_STRING("GattClientWriteCharacteristicValue"));

nit: WriteDescriptor

::: dom/bluetooth2/BluetoothGattDescriptor.h
@@ +23,5 @@
>  class BluetoothValue;
>  
>  class BluetoothGattDescriptor final : public nsISupports
> +                                    , public nsWrapperCache,
> +                                          public BluetoothSignalObserver

nit: , public BluetoothSignalObserver and align with #26.

@@ +72,5 @@
>  private:
>    ~BluetoothGattDescriptor();
>  
> +  /**
> +   * Update the this descriptor.

nit: Update the value of this descriptor.
Attachment #8583666 - Flags: feedback?(joliu)
(Assignee)

Comment 17

3 years ago
(In reply to jocelyn [:jocelyn] from comment #13)
> ::: dom/bluetooth2/BluetoothGattCharacteristic.cpp
> @@ +34,5 @@
> >    : mOwner(aOwner)
> >    , mService(aService)
> >    , mCharId(aCharId)
> > +  , mProperties(BLUETOOTH_EMPTY_GATT_CHAR_PROP)
> > +  , mWriteType(GATT_WRITE_TYPE_NO_RESPONSE)
> 
> Maybe we should remove DEFAULT keyword if the default write type is not
> GATT_WRITE_TYPE_DEFAULT?

I will rename GATT_WRITE_TYPE_DEFAULT as GATT_WRITE_TYPE_NORMAL and remove mWriteType member variable from class BluetoothGattCharacteristic.
(Reporter)

Comment 18

3 years ago
(In reply to Bruce Sun [:brsun] from comment #17)
> (In reply to jocelyn [:jocelyn] from comment #13)
> > ::: dom/bluetooth2/BluetoothGattCharacteristic.cpp
> > @@ +34,5 @@
> > >    : mOwner(aOwner)
> > >    , mService(aService)
> > >    , mCharId(aCharId)
> > > +  , mProperties(BLUETOOTH_EMPTY_GATT_CHAR_PROP)
> > > +  , mWriteType(GATT_WRITE_TYPE_NO_RESPONSE)
> > 
> > Maybe we should remove DEFAULT keyword if the default write type is not
> > GATT_WRITE_TYPE_DEFAULT?
> 
> I will rename GATT_WRITE_TYPE_DEFAULT as GATT_WRITE_TYPE_NORMAL and remove
> mWriteType member variable from class BluetoothGattCharacteristic.

WRITE_TYPE_WITH_RESPONSE sounds more specific to me, but I'm OK to both of them.
Totally depends on you.
(Assignee)

Comment 19

3 years ago
(In reply to jocelyn [:jocelyn] from comment #15)
> Comment on attachment 8583663 [details] [diff] [review]
> bug1140952_02_gatt_read_write_characteristic_ipc.patch
> 
> Review of attachment 8583663 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 1) Shouldn't we update the characteristic value after writeCharacteristic
> operation?
> 

According to the statement on [1], probably the answer is NO.

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGattCharacteristic#value

> 2) Do we need to save each characteristic/descriptor info even there's no
> read/write request on it?
>    Especially there is already one copy in the content process.
>    Have you consider handle this retry in content process(additional ipc
> cost) or just save a copy for ongoing requests?
> 

Currently gecko puts all GATT-related logics into BluetoothGattManager and keep content side as simple as possible. Retrying it from the content process could save the extra variables, but doing so might not fit with current design.

We have to queue all read/write parameters requested by users. If we only use one variable to save the requested value (maybe one for read and one for write), the value will be overwritten by the upcoming requests, and the originally unresolved request will be dropped silently.

There are two things we need to handle:
(1) correct and efficient mapping between GattId and the requested values.
(2) flow control of sequential protocol as stated on comment 5.

This patch only handles (1) but leaves (2) on bug 1147776. Because the design of (2) would re-design the solution of (1), how about addressing your concerns on bug 1147776?

> ::: dom/bluetooth2/BluetoothCommon.h
> @@ +618,5 @@
> >      return mId == aOther.mId && mIsPrimary == aOther.mIsPrimary;
> >    }
> >  };
> >  
> > +struct BluetoothGattCharTrait {
> 
> IMO, 'Trait' does not fit the use case here.
> Could you come up with another name to describe this?
> 

I am going to use BluetoothGattCharAttribute, what do you think?

> ::: dom/bluetooth2/BluetoothGattCharacteristic.cpp
> @@ +95,5 @@
> >    BluetoothValue v = aData.value();
> >    if (aData.name().EqualsLiteral("DescriptorsDiscovered")) {
> >      HandleDescriptorsDiscovered(v);
> > +  } else if (aData.name().EqualsLiteral("CharacteristicValueUpdated")) {
> > +    HandleCharacteristicValueUpdated(v);
> 
> nit: This function is quite trivial, how about we do it here directly?
> 

I would suggest to use a explicit function to handle sub-routine of each different notification to make codes clear, even the sub-routine is to handle trivial things. What do you think?

> @@ +225,5 @@
> > +  static_assert(sizeof(*aValue.Data()) == sizeof(uint8_t),
> > +                "byte-sized data required");
> > +
> > +  nsTArray<uint8_t> value;
> > +  value.AppendElements<uint8_t>(aValue.Data(), aValue.Length());
> 
> Do we need to add <uint8_t> here?
> 

It seems <class Item> can be substituted automatically. I will remove <uint8_t> from codes.

> @@ +735,5 @@
> > +
> > +  ENSURE_GATT_CLIENT_INTF_IS_READY_VOID(aRunnable);
> > +
> > +  size_t index = sClients->IndexOf(aAppUuid, 0 /* Start */, UuidComparator());
> > +  if (NS_WARN_IF(index == sClients->NoIndex)) {
> 
> Will this case happen here?
> ClientIf will only be unregistered in |BluetoothGatt::DisconnectFromOwner|
> and |BluetoothGatt::~BluetoothGatt|.
> Shouldn't we use MOZ_ASSERT in these kind of things?
> 

It would be a little weird from the class/function design if we use |MOZ_ASSERT| to check the valid value of the input parameter, |aAppUuid|:
 - There is no obvious limitations that |aAppUuid| cannot carry invalid value by anyone who uses |BluetoothGattManager::ReadCharacteristicValue()|.
 - There is no feasible ways to retrieve valid values explicitly from |BluetoothGattManager|.

Currently we don't have such use cases to pass an invalid value (ex. mismatched |nsAString|) just because we tightly couple many classes from the content process to the chrome process, but it might not a good behavior to let the fundamental class (ex. |BluetoothGattManager|) simply crash while encountering an unexpected (but not prohibited) input parameter.

What do you think?
Flags: needinfo?(joliu)
(Reporter)

Comment 20

3 years ago
(In reply to Bruce Sun [:brsun] from comment #19)
> (In reply to jocelyn [:jocelyn] from comment #15)
> > Comment on attachment 8583663 [details] [diff] [review]
> > bug1140952_02_gatt_read_write_characteristic_ipc.patch
> > 
> > Review of attachment 8583663 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > 1) Shouldn't we update the characteristic value after writeCharacteristic
> > operation?
> > 
> 
> According to the statement on [1], probably the answer is NO.
> 
> [1]
> https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/
> BluetoothGattCharacteristic#value
> 

Got it, OK, let's follow it since proposed WebBluetooth API also doesn't specify this behavior.
https://webbluetoothcg.github.io/web-bluetooth/#widl-BluetoothGATTCharacteristic-value

> > 2) Do we need to save each characteristic/descriptor info even there's no
> > read/write request on it?
> >    Especially there is already one copy in the content process.
> >    Have you consider handle this retry in content process(additional ipc
> > cost) or just save a copy for ongoing requests?
> > 
> 
> Currently gecko puts all GATT-related logics into BluetoothGattManager and
> keep content side as simple as possible. Retrying it from the content
> process could save the extra variables, but doing so might not fit with
> current design.
> 
> We have to queue all read/write parameters requested by users. If we only
> use one variable to save the requested value (maybe one for read and one for
> write), the value will be overwritten by the upcoming requests, and the
> originally unresolved request will be dropped silently.

Yes, that's correct.
Actually I meant a data structure containing all ongoing requests instead of just one.
Sorry if I didn't make it clear.

> 
> There are two things we need to handle:
> (1) correct and efficient mapping between GattId and the requested values.

You already have a mapping in your patch, right?
Couldn't we just put into the map when requested?

> (2) flow control of sequential protocol as stated on comment 5.
> 
> This patch only handles (1) but leaves (2) on bug 1147776. Because the
> design of (2) would re-design the solution of (1), how about addressing your
> concerns on bug 1147776?
> 
> > ::: dom/bluetooth2/BluetoothCommon.h
> > @@ +618,5 @@
> > >      return mId == aOther.mId && mIsPrimary == aOther.mIsPrimary;
> > >    }
> > >  };
> > >  
> > > +struct BluetoothGattCharTrait {
> > 
> > IMO, 'Trait' does not fit the use case here.
> > Could you come up with another name to describe this?
> > 
> 
> I am going to use BluetoothGattCharAttribute, what do you think?
> 

I'm OK with it.

> > ::: dom/bluetooth2/BluetoothGattCharacteristic.cpp
> > @@ +95,5 @@
> > >    BluetoothValue v = aData.value();
> > >    if (aData.name().EqualsLiteral("DescriptorsDiscovered")) {
> > >      HandleDescriptorsDiscovered(v);
> > > +  } else if (aData.name().EqualsLiteral("CharacteristicValueUpdated")) {
> > > +    HandleCharacteristicValueUpdated(v);
> > 
> > nit: This function is quite trivial, how about we do it here directly?
> > 
> 
> I would suggest to use a explicit function to handle sub-routine of each
> different notification to make codes clear, even the sub-routine is to
> handle trivial things. What do you think?
> 

I'm OK with it.

> > @@ +735,5 @@
> > > +
> > > +  ENSURE_GATT_CLIENT_INTF_IS_READY_VOID(aRunnable);
> > > +
> > > +  size_t index = sClients->IndexOf(aAppUuid, 0 /* Start */, UuidComparator());
> > > +  if (NS_WARN_IF(index == sClients->NoIndex)) {
> > 
> > Will this case happen here?
> > ClientIf will only be unregistered in |BluetoothGatt::DisconnectFromOwner|
> > and |BluetoothGatt::~BluetoothGatt|.
> > Shouldn't we use MOZ_ASSERT in these kind of things?
> > 
> 
> It would be a little weird from the class/function design if we use
> |MOZ_ASSERT| to check the valid value of the input parameter, |aAppUuid|:
>  - There is no obvious limitations that |aAppUuid| cannot carry invalid
> value by anyone who uses |BluetoothGattManager::ReadCharacteristicValue()|.
>  - There is no feasible ways to retrieve valid values explicitly from
> |BluetoothGattManager|.
> 
> Currently we don't have such use cases to pass an invalid value (ex.
> mismatched |nsAString|) just because we tightly couple many classes from the
> content process to the chrome process, but it might not a good behavior to
> let the fundamental class (ex. |BluetoothGattManager|) simply crash while
> encountering an unexpected (but not prohibited) input parameter.
> 
> What do you think?

Got your point.
To be consistent, could we open a followup bug for revising them all together instead of doing it here?
Flags: needinfo?(joliu)
(Assignee)

Comment 21

3 years ago
Created attachment 8589036 [details] [diff] [review]
bug1140952_01_gatt_read_write_characteristic_webapi.v2.patch

Address comment 13
Attachment #8583662 - Attachment is obsolete: true
Attachment #8589036 - Flags: review?(shuang)
Attachment #8589036 - Flags: feedback?(joliu)
(Assignee)

Comment 22

3 years ago
Created attachment 8589039 [details] [diff] [review]
bug1140952_02_gatt_read_write_characteristic_ipc.v2.patch

Address comment 15.

I remove the hash-table since bug 1147776 is going to use different way to handle the issues mentioned on comment 5.
Attachment #8583663 - Attachment is obsolete: true
Attachment #8589039 - Flags: review?(shuang)
Attachment #8589039 - Flags: feedback?(joliu)
(Assignee)

Comment 23

3 years ago
Comment on attachment 8583664 [details] [diff] [review]
bug1140952_03_gatt_read_write_descriptor_webapi.patch

Request for review.
Attachment #8583664 - Flags: review?(shuang)
(Assignee)

Comment 24

3 years ago
Created attachment 8589042 [details] [diff] [review]
bug1140952_04_gatt_read_write_descriptor_ipc.v2.patch

Address comment 16.
Attachment #8583666 - Attachment is obsolete: true
Attachment #8589042 - Flags: review?(shuang)
Attachment #8589042 - Flags: feedback?(joliu)
(Reporter)

Comment 25

3 years ago
Comment on attachment 8589036 [details] [diff] [review]
bug1140952_01_gatt_read_write_characteristic_webapi.v2.patch

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

f=me with comment addressed, thanks!

::: dom/bluetooth2/BluetoothGattCharacteristic.cpp
@@ +128,5 @@
> +
> +already_AddRefed<Promise>
> +BluetoothGattCharacteristic::ReadValue(ErrorResult& aRv)
> +{
> +  return nullptr;

nit: 
Suggest to leave a temporary comment that this will be implemented by later patch set in the same bug, since it doesn't match the webidl.

@@ +135,5 @@
> +already_AddRefed<Promise>
> +BluetoothGattCharacteristic::WriteValue(const ArrayBuffer& aValue,
> +                                        ErrorResult& aRv)
> +{
> +  return nullptr;

DITTO.

::: dom/bluetooth2/BluetoothGattCharacteristic.h
@@ +18,5 @@
>  #include "nsPIDOMWindow.h"
>  
> +namespace mozilla {
> +namespace dom {
> +struct Properties;

Do we still need this if you already include BluetoothGattCharacteristicBinding.h?
Attachment #8589036 - Flags: feedback?(joliu) → feedback+
(Assignee)

Comment 26

3 years ago
(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #25)
> Suggest to leave a temporary comment that this will be implemented by later
> patch set in the same bug, since it doesn't match the webidl.

Thanks. I will add a temporary comment for it.

> Do we still need this if you already include
> BluetoothGattCharacteristicBinding.h?

I will remove this struct declaration.
Comment on attachment 8589036 [details] [diff] [review]
bug1140952_01_gatt_read_write_characteristic_webapi.v2.patch

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

::: dom/bluetooth2/BluetoothCommon.h
@@ +556,5 @@
> +  GATT_WRITE_TYPE_PREPARE,
> +  GATT_WRITE_TYPE_SIGNED
> +};
> +
> +enum BluetoothGattCharPropBit {

Add Characteristic Properties bit field.

@@ +565,5 @@
> +  GATT_CHAR_PROP_BIT_NOTIFY               = (1 << 4),
> +  GATT_CHAR_PROP_BIT_INDICATE             = (1 << 5),
> +  GATT_CHAR_PROP_BIT_SIGNED_WRITE         = (1 << 6),
> +  GATT_CHAR_PROP_BIT_EXTENDED_PROPERTIES  = (1 << 7)
> +};

nit: new line here

::: dom/bluetooth2/BluetoothGattCharacteristic.cpp
@@ +113,5 @@
> +  return;
> +}
> +
> +void
> +BluetoothGattCharacteristic::GetProperties(mozilla::dom::Properties& aProperties) const

Can we rename to GattCharacteristicProperties? mozilla::dom:Properties is easy to hit naming conflict.

::: dom/bluetooth2/BluetoothGattCharacteristic.h
@@ +18,5 @@
>  #include "nsPIDOMWindow.h"
>  
> +namespace mozilla {
> +namespace dom {
> +struct Properties;

I think we don't need it if we include BluetoothGAttCharacteristicBinding, but calling Properties is not a good idea, I'm afraid of naming conflict.

::: dom/webidl/BluetoothGattCharacteristic.webidl
@@ +3,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +dictionary Properties

Can we rename to GattCharacteristicProperties?
Attachment #8589036 - Flags: review?(shuang) → review-
(Assignee)

Comment 28

3 years ago
Created attachment 8589506 [details] [diff] [review]
bug1140952_01_gatt_read_write_characteristic_webapi.v3.patch

Address comment 27.
Attachment #8589036 - Attachment is obsolete: true
Attachment #8589506 - Flags: review?(shuang)
Comment on attachment 8589506 [details] [diff] [review]
bug1140952_01_gatt_read_write_characteristic_webapi.v3.patch

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

It looks good to me.

::: dom/bluetooth2/BluetoothGattCharacteristic.cpp
@@ +33,5 @@
>    const BluetoothGattId& aCharId)
>    : mOwner(aOwner)
>    , mService(aService)
>    , mCharId(aCharId)
> +  , mProperties(BLUETOOTH_EMPTY_GATT_CHAR_PROP)

This part will be implemented in the later patch. So, I will ignore this empty CHAR_PROP.

@@ +34,5 @@
>    : mOwner(aOwner)
>    , mService(aService)
>    , mCharId(aCharId)
> +  , mProperties(BLUETOOTH_EMPTY_GATT_CHAR_PROP)
> +  , mWriteType(GATT_WRITE_TYPE_NORMAL)

This part will be implemented in the later patch. So, I will ignore this default type.
Attachment #8589506 - Flags: review?(shuang) → review+
(Reporter)

Comment 30

3 years ago
Comment on attachment 8589039 [details] [diff] [review]
bug1140952_02_gatt_read_write_characteristic_ipc.v2.patch

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

Hi Bruce,

Please see my comments below.

Thanks,
Jocelyn

::: dom/bluetooth2/bluedroid/BluetoothGattManager.cpp
@@ +108,5 @@
> +  BluetoothGattWriteType mCharacteristicWriteType;
> +  nsTArray<uint8_t> mCharacteristicWriteValue;
> +  bool mWriteCharacteristicAuthRetry;
> +  nsRefPtr<BluetoothReplyRunnable> mWriteCharacteristicRunnable;
> +

nit: Suggest to put some comments for #105-#111

@@ +671,5 @@
> +    NS_ENSURE_TRUE_VOID(bs);
> +
> +    // Reject the read characteristic value request
> +    DispatchReplyError(runnable,
> +                       NS_LITERAL_STRING("ReadCharacteristicValue failed"));

How about we put the clean up part after reject request and remove runnable variable?

@@ +700,5 @@
> +
> +  nsRefPtr<BluetoothGattClient> client = sClients->ElementAt(index);
> +
> +  client->mReadCharacteristicAuthRetry = false;
> +  client->mReadCharacteristicRunnable = aRunnable;

If there's an ongoing request, we shouldn't overwrite it silently here.
Please reject the Promise here and put a comment here to state that why we couldn't handle multiple read requests and put the follow-up bug number in the comment.

@@ +771,5 @@
> +
> +  client->mCharacteristicWriteType = aWriteType;
> +  client->mCharacteristicWriteValue = aValue;
> +  client->mWriteCharacteristicAuthRetry = false;
> +  client->mWriteCharacteristicRunnable = aRunnable;

DITTO.

@@ +1181,5 @@
> +  nsRefPtr<BluetoothReplyRunnable> runnable =
> +    client->mReadCharacteristicRunnable;
> +
> +  if (aStatus == GATT_STATUS_SUCCESS) {
> +    client->mReadCharacteristicAuthRetry = false;

Could we put these reset actions into a function of BluetoothGattClient and use that while resetting?
It will be easier to make sure we didn't miss anything.

@@ +1231,5 @@
> +  nsRefPtr<BluetoothGattClient> client = sClients->ElementAt(index);
> +
> +  MOZ_ASSERT(client->mWriteCharacteristicRunnable);
> +  nsRefPtr<BluetoothReplyRunnable> runnable =
> +    client->mWriteCharacteristicRunnable;

nit: suggest to use client->mWriteCharacteristicRunnable below to be consistent with usage of AuthRetry and others.

@@ +1237,5 @@
> +  if (aStatus == GATT_STATUS_SUCCESS) {
> +    client->mCharacteristicWriteType = GATT_WRITE_TYPE_NORMAL;
> +    client->mCharacteristicWriteValue.Clear();
> +    client->mWriteCharacteristicAuthRetry = false;
> +    client->mWriteCharacteristicRunnable = nullptr;

DITTO. Could we make it a member function of BluetoothGattClient?

::: dom/bluetooth2/bluez/BluetoothDBusService.cpp
@@ +4301,5 @@
>  BluetoothDBusService::UnregisterGattClientInternal(
>    int aClientIf, BluetoothReplyRunnable* aRunnable)
>  {
>  }
>  

why is it removed?

::: dom/bluetooth2/ipc/BluetoothParent.cpp
@@ +773,5 @@
> +  MOZ_ASSERT(mRequestType ==
> +             Request::TGattClientReadCharacteristicValueRequest);
> +
> +  mService->GattClientReadCharacteristicValueInternal(aRequest.appUuid(),
> +                                                      aRequest.serviceId(),

nit: put in the next line.
And same as the function below.

::: dom/bluetooth2/ipc/BluetoothServiceChildProcess.cpp
@@ +429,5 @@
> +  const BluetoothGattId& aCharacteristicId,
> +  BluetoothReplyRunnable* aRunnable)
> +{
> +  SendRequest(aRunnable,
> +    GattClientReadCharacteristicValueRequest(nsString(aAppUuid), aServiceId,

nit: put aServiceId in the next line.

@@ +443,5 @@
> +  const nsTArray<uint8_t>& aValue,
> +  BluetoothReplyRunnable* aRunnable)
> +{
> +  SendRequest(aRunnable,
> +      GattClientWriteCharacteristicValueRequest(nsString(aAppUuid), aServiceId,

DITTO.

@@ +444,5 @@
> +  BluetoothReplyRunnable* aRunnable)
> +{
> +  SendRequest(aRunnable,
> +      GattClientWriteCharacteristicValueRequest(nsString(aAppUuid), aServiceId,
> +                                                aCharacteristicId, aWriteType,

DITTO.

::: dom/bluetooth2/ipc/BluetoothTypes.ipdlh
@@ +5,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +using mozilla::dom::bluetooth::BluetoothGattCharAttribute
> +  from "mozilla/dom/bluetooth/BluetoothCommon.h";
> +using mozilla::dom::bluetooth::BluetoothGattWriteType

nit: alphabet order
Attachment #8589039 - Flags: feedback?(joliu)
(Reporter)

Comment 31

3 years ago
Comment on attachment 8589042 [details] [diff] [review]
bug1140952_04_gatt_read_write_descriptor_ipc.v2.patch

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

Please see my comments below.
I didn't look into BluetoothGattManager.cpp though since you might need to revise it to address Comment30.
Will look it after that.

Thanks,
Jocelyn

::: dom/bluetooth2/BluetoothGattDescriptor.cpp
@@ +188,5 @@
> +  NS_ENSURE_TRUE(!aRv.Failed(), nullptr);
> +
> +  Properties properties;
> +  mCharacteristic->GetProperties(properties);
> +  if (!properties.mWrite) {

Shouldn't we check mWriteNoResponse and mSignedWrite also?

::: dom/bluetooth2/bluez/BluetoothDBusService.cpp
@@ +4302,5 @@
>    int aClientIf, BluetoothReplyRunnable* aRunnable)
>  {
>  }
>  
> +void

Is it because you remove it in the previous patch?
Suggest to add it back in the previous patch. ;)

::: dom/bluetooth2/ipc/BluetoothServiceChildProcess.cpp
@@ +471,5 @@
> +  const nsTArray<uint8_t>& aValue,
> +  BluetoothReplyRunnable* aRunnable)
> +{
> +  SendRequest(aRunnable,
> +      GattClientWriteDescriptorValueRequest(nsString(aAppUuid), aServiceId,

nit: two space indent here.
Attachment #8589042 - Flags: feedback?(joliu)
(Assignee)

Comment 32

3 years ago
Hi Jocelyn,

Thanks for the feedback.

> @@ +671,5 @@
> > +    NS_ENSURE_TRUE_VOID(bs);
> > +
> > +    // Reject the read characteristic value request
> > +    DispatchReplyError(runnable,
> > +                       NS_LITERAL_STRING("ReadCharacteristicValue failed"));
> 
> How about we put the clean up part after reject request and remove runnable
> variable?
> 

Perhaps these member variables should be cleaned up immediately while |OnError| is called. Although |DispatchReplyError| is very simple and it will not touch any member variables of |BluetoothGattClient|, it would be good to cache the necessary value explicitly in local (ex. the runnable object) and to reset those member variables before performing other tasks (ex. any error handling). What do you think?

> ::: dom/bluetooth2/bluez/BluetoothDBusService.cpp
> @@ +4301,5 @@
> >  BluetoothDBusService::UnregisterGattClientInternal(
> >    int aClientIf, BluetoothReplyRunnable* aRunnable)
> >  {
> >  }
> >  
> 
> why is it removed?
> 

I will add it back.

> ::: dom/bluetooth2/ipc/BluetoothParent.cpp
> @@ +773,5 @@
> > +  MOZ_ASSERT(mRequestType ==
> > +             Request::TGattClientReadCharacteristicValueRequest);
> > +
> > +  mService->GattClientReadCharacteristicValueInternal(aRequest.appUuid(),
> > +                                                      aRequest.serviceId(),
> 
> nit: put in the next line.
> And same as the function below.
> 

I am not sure I understand your comments correctly. Would you mind helping to comment more about which one is suggested to be put in the next line?
(Assignee)

Comment 33

3 years ago
> ::: dom/bluetooth2/BluetoothGattDescriptor.cpp
> @@ +188,5 @@
> > +  NS_ENSURE_TRUE(!aRv.Failed(), nullptr);
> > +
> > +  Properties properties;
> > +  mCharacteristic->GetProperties(properties);
> > +  if (!properties.mWrite) {
> 
> Shouldn't we check mWriteNoResponse and mSignedWrite also?
> 

According to GATT spec, there are only two types of writing for descriptors. One is described in "4.12.3 Write Characteristic Descriptors", another one is described in "4.12.4 Write Long Characteristic Descriptors". There is no signed_write or write_with_no_response for descriptors. So those two boolean values on characteristics might not be so related to descriptors.
(Assignee)

Comment 34

3 years ago
Created attachment 8589622 [details] [diff] [review]
bug1140952_02_gatt_read_write_characteristic_ipc.v3.patch

Address comment 30 expect for unconfirmed items on comment 32.
Attachment #8589039 - Attachment is obsolete: true
Attachment #8589039 - Flags: review?(shuang)
Attachment #8589622 - Flags: review?(shuang)
Attachment #8589622 - Flags: feedback?(joliu)
(Assignee)

Comment 35

3 years ago
Created attachment 8589624 [details] [diff] [review]
bug1140952_04_gatt_read_write_descriptor_ipc.v3.patch

Address comment 31.
Attachment #8589042 - Attachment is obsolete: true
Attachment #8589042 - Flags: review?(shuang)
Attachment #8589624 - Flags: review?(shuang)
Attachment #8589624 - Flags: feedback?(joliu)
(Reporter)

Comment 36

3 years ago
(In reply to Bruce Sun [:brsun] from comment #32)
> Hi Jocelyn,
> 
> Thanks for the feedback.
> 
> > @@ +671,5 @@
> > > +    NS_ENSURE_TRUE_VOID(bs);
> > > +
> > > +    // Reject the read characteristic value request
> > > +    DispatchReplyError(runnable,
> > > +                       NS_LITERAL_STRING("ReadCharacteristicValue failed"));
> > 
> > How about we put the clean up part after reject request and remove runnable
> > variable?
> > 
> 
> Perhaps these member variables should be cleaned up immediately while
> |OnError| is called. Although |DispatchReplyError| is very simple and it
> will not touch any member variables of |BluetoothGattClient|, it would be
> good to cache the necessary value explicitly in local (ex. the runnable
> object) and to reset those member variables before performing other tasks
> (ex. any error handling). What do you think?
> 

Looks like we wouldn't touch them in our use cases.
Personally prefer clean up later for fewer code and consistency.
I have no strong opinion on this though, you may stay your original design if you want.

> > ::: dom/bluetooth2/bluez/BluetoothDBusService.cpp
> > @@ +4301,5 @@
> > >  BluetoothDBusService::UnregisterGattClientInternal(
> > >    int aClientIf, BluetoothReplyRunnable* aRunnable)
> > >  {
> > >  }
> > >  
> > 
> > why is it removed?
> > 
> 
> I will add it back.
> 
> > ::: dom/bluetooth2/ipc/BluetoothParent.cpp
> > @@ +773,5 @@
> > > +  MOZ_ASSERT(mRequestType ==
> > > +             Request::TGattClientReadCharacteristicValueRequest);
> > > +
> > > +  mService->GattClientReadCharacteristicValueInternal(aRequest.appUuid(),
> > > +                                                      aRequest.serviceId(),
> > 
> > nit: put in the next line.
> > And same as the function below.
> > 
> 
> I am not sure I understand your comments correctly. Would you mind helping
> to comment more about which one is suggested to be put in the next line?

Sorry, my browser has a strange layout on it, please ignore this comment.
(Reporter)

Comment 37

3 years ago
Comment on attachment 8589622 [details] [diff] [review]
bug1140952_02_gatt_read_write_characteristic_ipc.v3.patch

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

f=me with comments addressed.

Thanks for the effort!

::: dom/bluetooth2/BluetoothGattCharacteristic.cpp
@@ +179,4 @@
>  already_AddRefed<Promise>
>  BluetoothGattCharacteristic::ReadValue(ErrorResult& aRv)
>  {
>    // TODO: This will be implemented by later patch set in the same bug.

remove the TODO comment.

::: dom/bluetooth2/bluedroid/BluetoothGattManager.cpp
@@ +43,5 @@
> +{
> +  bool mAuthRetry;
> +  nsRefPtr<BluetoothReplyRunnable> mRunnable;
> +
> +  void Reset(bool aAuthRetry,

Reset is a bit confusing, suggest to use something similar as 'Set'.

@@ +64,5 @@
> +  nsTArray<uint8_t> mWriteValue;
> +  bool mAuthRetry;
> +  nsRefPtr<BluetoothReplyRunnable> mRunnable;
> +
> +  void Reset(BluetoothGattWriteType aWriteType,

DITTO.

@@ +163,5 @@
> +   * value is handled in the same time. Theoretically this request can be
> +   * interfered by other kind of requests as well, but that problem is going to
> +   * be handled on bug 1147776.
> +   */
> +  BluetoothGattClientWriteCharState mWriteCharacteristicState;

Suggest to group these two states and keep one comment only.
And I think this comment doesn't describe what these two states for.
Please revise it.
Attachment #8589622 - Flags: feedback?(joliu) → feedback+
(Reporter)

Comment 38

3 years ago
Comment on attachment 8589624 [details] [diff] [review]
bug1140952_04_gatt_read_write_descriptor_ipc.v3.patch

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

f=me with comments addressed, thanks!

::: dom/bluetooth2/bluedroid/BluetoothGattManager.cpp
@@ +89,5 @@
> +{
> +  bool mAuthRetry;
> +  nsRefPtr<BluetoothReplyRunnable> mRunnable;
> +
> +  void Reset(bool aAuthRetry,

Same as Comment 37.

@@ +225,5 @@
> +   * value is handled in the same time. Theoretically this request can be
> +   * interfered by other kind of requests as well, but that problem is going to
> +   * be handled on bug 1147776.
> +   */
> +  BluetoothGattClientWriteDescState mWriteDescriptorState;

Same as Comment 37, please group these four together and revise the comment.
Attachment #8589624 - Flags: feedback?(joliu) → feedback+
Attachment #8583664 - Flags: review?(shuang) → review?(btian)
Attachment #8589622 - Flags: review?(shuang) → review?(btian)
Attachment #8589624 - Flags: review?(shuang) → review?(btian)
Ben is back! Transfer API v2 review to him. :) Good to see you come back!

Comment 40

3 years ago
Comment on attachment 8589622 [details] [diff] [review]
bug1140952_02_gatt_read_write_characteristic_ipc.v3.patch

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

Please see my comment below.

::: dom/bluetooth2/BluetoothGattCharacteristic.cpp
@@ +170,5 @@
> +    }
> +
> +    return true;
> +  }
> +

Override |ReleaseMembers| to release |mCharacteristic| as [1].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/BluetoothAdapter.cpp#118

@@ +188,5 @@
> +
> +  nsRefPtr<Promise> promise = Promise::Create(global, aRv);
> +  NS_ENSURE_TRUE(!aRv.Failed(), nullptr);
> +
> +  if (!(mProperties & GATT_CHAR_PROP_BIT_READ)) {

Suggest to simplify as

  BT_ENSURE_TRUE_REJECT(mProperties & GATT_CHAR_PROP_BIT_READ, NS_ERROR_NOT_AVAILABLE);

@@ +218,5 @@
> +
> +  nsRefPtr<Promise> promise = Promise::Create(global, aRv);
> +  NS_ENSURE_TRUE(!aRv.Failed(), nullptr);
> +
> +  if (!(mProperties & GATT_CHAR_PROP_BIT_WRITE_NO_RESPONSE) &&

Suggest to simplify as

  BT_ENSURE_TRUE_REJECT(mProperties &
                          (GATT_CHAR_PROP_BIT_WRITE_NO_RESPONSE &
                           GATT_CHAR_PROP_BIT_WRITE &
                           GATT_CHAR_PROP_BIT_SIGNED_WRITE),
                        NS_ERROR_NOT_AVAILABLE);

@@ +227,5 @@
> +  }
> +
> +  aValue.ComputeLengthAndData();
> +
> +  static_assert(sizeof(*aValue.Data()) == sizeof(uint8_t),

Why use |static_assert| here? Also why convert ArrayBuffer to uint8_t array instead of using ArrayBuffer all around?

::: dom/bluetooth2/BluetoothGattCharacteristic.h
@@ +102,5 @@
>  
> +  /**
> +   * Update the value of this characteristic.
> +   *
> +   * @param aValue [in] BluetoothValue which contains an array of uint8_t

Simplify to "which contains an uint8_t array."

::: dom/bluetooth2/bluedroid/BluetoothGattManager.cpp
@@ +53,5 @@
> +
> +  void Reset()
> +  {
> +    mAuthRetry = false;
> +    mRunnable = nullptr;

Replace with |Set(false, nullptr)|.

@@ +150,5 @@
>  
> +  /*
> +   * Concurrent requests would be dropped silently if the sequential protocol
> +   * is not respected properly. So only one request of reading a characteristic
> +   * value is handled in the same time. Theoretically this request can be

nit: 'at' the same time.

@@ +719,5 @@
> +    MOZ_ASSERT(mClient->mReadCharacteristicState.mRunnable);
> +
> +    nsRefPtr<BluetoothReplyRunnable> runnable =
> +      mClient->mReadCharacteristicState.mRunnable;
> +    mClient->mReadCharacteristicState.Reset();

Why not reset after rejecting promise? So that you don't have to remember |mClient->mReadCharacteristicState.mRunnable|.

@@ +1262,5 @@
> +
> +    BluetoothSignal signal(NS_LITERAL_STRING("CharacteristicValueUpdated"),
> +                           path,
> +                           BluetoothValue(value));
> +    bs->DistributeSignal(signal);

Simplify to 

  bs->DistributeSignal(NS_LITERAL_STRING("CharacteristicValueUpdated"),
                       path,
                       BluetoothValue(value));

::: dom/bluetooth2/bluedroid/BluetoothHALHelpers.h
@@ +804,5 @@
> +inline nsresult
> +Convert(int aIn, BluetoothGattCharProp& aOut)
> +{
> +  /* Reference: $B2G/external/bluetooth/bluedroid/stack/include/gatt_api.h */
> +  aOut = BLUETOOTH_EMPTY_GATT_CHAR_PROP;

Can we set |aOut| directly as following?

  aOut = static_cast<BluetoothGattCharProp>(aIn & 0xFF);
Attachment #8589622 - Flags: review?(btian)
(Assignee)

Comment 41

3 years ago
> Got your point.
> To be consistent, could we open a followup bug for revising them all
> together instead of doing it here?

Create bug 1153718 for follow up.
(Assignee)

Comment 42

3 years ago
Thanks for reviewing. I will modify most of them correspondingly. Please see my comments below.

> @@ +227,5 @@
> > +  }
> > +
> > +  aValue.ComputeLengthAndData();
> > +
> > +  static_assert(sizeof(*aValue.Data()) == sizeof(uint8_t),
> 
> Why use |static_assert| here? Also why convert ArrayBuffer to uint8_t array
> instead of using ArrayBuffer all around?
> 

Some existing codes use |static_assert| as a convention while using |ArrayBuffer| (ex. [1], [2]). This check might not so necessary because |ArrayBuffer| uses |uint8_t| by definition [3]. I will remove this checking if it is suggested.

|uint8_t[]| in IPDL can be translated as |nsTArray<uint8_t>| directly [4]. |ArrayBuffer| seems an array container used just for dom binding. Using |nsTArray<uint8_t>| is intuitive for me. Any reason |ArrayBuffer| is preferred?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/WebSocket.cpp#2247
[2] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDOMDataChannel.cpp#303
[3] https://dxr.mozilla.org/mozilla-central/source/dom/bindings/TypedArray.h#222
[4] https://developer.mozilla.org/en-US/docs/IPDL/Tutorial#Arrays

> ::: dom/bluetooth2/bluedroid/BluetoothGattManager.cpp
> @@ +53,5 @@
> > +
> > +  void Reset()
> > +  {
> > +    mAuthRetry = false;
> > +    mRunnable = nullptr;
> 
> Replace with |Set(false, nullptr)|.
> 

I would prefer to have one explicitly reset function. Otherwise it might need to pass one temporary empty |nsTArray| for write-state.

> @@ +719,5 @@
> > +    MOZ_ASSERT(mClient->mReadCharacteristicState.mRunnable);
> > +
> > +    nsRefPtr<BluetoothReplyRunnable> runnable =
> > +      mClient->mReadCharacteristicState.mRunnable;
> > +    mClient->mReadCharacteristicState.Reset();
> 
> Why not reset after rejecting promise? So that you don't have to remember
> |mClient->mReadCharacteristicState.mRunnable|.
> 

The reason is stated on comment 32. Bug 1153722 is also created for syncing the coding style to avoid holding wasted resource in the manager.

> ::: dom/bluetooth2/bluedroid/BluetoothHALHelpers.h
> @@ +804,5 @@
> > +inline nsresult
> > +Convert(int aIn, BluetoothGattCharProp& aOut)
> > +{
> > +  /* Reference: $B2G/external/bluetooth/bluedroid/stack/include/gatt_api.h */
> > +  aOut = BLUETOOTH_EMPTY_GATT_CHAR_PROP;
> 
> Can we set |aOut| directly as following?
> 
>   aOut = static_cast<BluetoothGattCharProp>(aIn & 0xFF);

I am afraid that this might not be a good way to isolate our own definition from the underlying Bluetooth stack by performing bit-wise-and with 0xFF directly. Types and values defined in BluetoothCommon.h are used to decouple the dependency between pre-defined values in Bluetooth stack and Gecko-defined values. It would be better not to assume the 1-1 mapping relation of bit positions. What do you think?
(Assignee)

Comment 43

3 years ago
Add ni? for comment 42.
Flags: needinfo?(btian)

Comment 44

3 years ago
(In reply to Bruce Sun [:brsun] from comment #42)
> > Why use |static_assert| here? Also why convert ArrayBuffer to uint8_t array
> > instead of using ArrayBuffer all around?
> > 
> 
> Some existing codes use |static_assert| as a convention while using
> |ArrayBuffer| (ex. [1], [2]). This check might not so necessary because
> |ArrayBuffer| uses |uint8_t| by definition [3]. I will remove this checking
> if it is suggested.

Suggest to remove since it seems redundant and confusing.

> |uint8_t[]| in IPDL can be translated as |nsTArray<uint8_t>| directly [4].
> |ArrayBuffer| seems an array container used just for dom binding. Using
> |nsTArray<uint8_t>| is intuitive for me. Any reason |ArrayBuffer| is
> preferred?

Got it. So the reason is for IPC argument passing.

> > ::: dom/bluetooth2/bluedroid/BluetoothGattManager.cpp
> > @@ +53,5 @@
> > > +
> > > +  void Reset()
> > > +  {
> > > +    mAuthRetry = false;
> > > +    mRunnable = nullptr;
> > 
> > Replace with |Set(false, nullptr)|.
> > 
> 
> I would prefer to have one explicitly reset function. Otherwise it might
> need to pass one temporary empty |nsTArray| for write-state.

Agree. It's more consistent at the cost of redundancy.
 
> > @@ +719,5 @@
> > > +    MOZ_ASSERT(mClient->mReadCharacteristicState.mRunnable);
> > > +
> > > +    nsRefPtr<BluetoothReplyRunnable> runnable =
> > > +      mClient->mReadCharacteristicState.mRunnable;
> > > +    mClient->mReadCharacteristicState.Reset();
> > 
> > Why not reset after rejecting promise? So that you don't have to remember
> > |mClient->mReadCharacteristicState.mRunnable|.
> > 
> 
> The reason is stated on comment 32. Bug 1153722 is also created for syncing
> the coding style to avoid holding wasted resource in the manager.

Got it.

> > ::: dom/bluetooth2/bluedroid/BluetoothHALHelpers.h
> > @@ +804,5 @@
> > > +inline nsresult
> > > +Convert(int aIn, BluetoothGattCharProp& aOut)
> > > +{
> > > +  /* Reference: $B2G/external/bluetooth/bluedroid/stack/include/gatt_api.h */
> > > +  aOut = BLUETOOTH_EMPTY_GATT_CHAR_PROP;
> > 
> > Can we set |aOut| directly as following?
> > 
> >   aOut = static_cast<BluetoothGattCharProp>(aIn & 0xFF);
> 
> I am afraid that this might not be a good way to isolate our own definition
> from the underlying Bluetooth stack by performing bit-wise-and with 0xFF
> directly. Types and values defined in BluetoothCommon.h are used to decouple
> the dependency between pre-defined values in Bluetooth stack and
> Gecko-defined values. It would be better not to assume the 1-1 mapping
> relation of bit positions. What do you think?

I see. But we can still simplify the code with loop as following:

  static const BluetoothGattCharPropBit sGattCharPropBit[] = {
    CONVERT(0, GATT_CHAR_PROP_BIT_BROADCAST),
    CONVERT(1, GATT_CHAR_PROP_BIT_READ),
    ...
    CONVERT(7, GATT_CHAR_PROP_BIT_EXTENDED_PROPERTIES)
  }

  aOut = BLUETOOTH_EMPTY_GATT_CHAR_PROP;
  for (uint8_t i = 0; i < MOZ_ARRAY_LENGTH(sGattCharPropBit); i++) {
    if (aIn & (1 << i)) {
      aOut |= sGattCharPropBit[i];
    }
  }
Flags: needinfo?(btian)
(Assignee)

Comment 45

3 years ago
Created attachment 8591543 [details] [diff] [review]
bug1140952_02_gatt_read_write_characteristic_ipc.v4.patch

Address comment 40 and comment 44.

One end-guard of |BluetoothGattWriteType| is added to correct the usage of |ContiguousEnumSerializer|.
Attachment #8589622 - Attachment is obsolete: true
Attachment #8591543 - Flags: review?(btian)
(Assignee)

Comment 46

3 years ago
Created attachment 8591544 [details] [diff] [review]
bug1140952_04_gatt_read_write_descriptor_ipc.v4.patch

Address similar issues of comment 40 for descriptor implementation.
Attachment #8589624 - Attachment is obsolete: true
Attachment #8589624 - Flags: review?(btian)
Attachment #8591544 - Flags: review?(btian)

Comment 47

3 years ago
Comment on attachment 8591543 [details] [diff] [review]
bug1140952_02_gatt_read_write_characteristic_ipc.v4.patch

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

r=me with comment addressed.

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +892,5 @@
> +   * is not respected properly. So only one request of reading a characteristic
> +   * value is handled at same time. Theoretically this request can be interfered
> +   * by other kind of requests as well, but that problem is going to be handled
> +   * on bug 1147776.
> +   */

Suggest to simplify as following:

  /**
   * Reject subsequent reading requests to follow ATT sequential protocol that
   * handles one request at a time. Otherwise underlying layers would drop the
   * subsequent requests silently.
   *  
   * Bug 1147776 intends to solve a larger problem that other kind of requests
   * may still interfere the ongoing request.
   */

@@ +906,5 @@
> +   * Try to read the characteristic value through a non-authenticated physical
> +   * link at the beginning. If the operation is failed due to insufficient
> +   * authentication or insufficient encryption key size, we try to read it
> +   * through an authenticated physical link again.
> +   */

Suggest to simplify as following:

  /**
   * First, read the characteristic value through an unauthenticated physical
   * link. If the operation fails due to insufficient authentication/encryption
   * key size, retry to read through an authenticated physical link.
   */

@@ +974,5 @@
> +   * is not respected properly. So only one request of writing a characteristic
> +   * value is handled at same time. Theoretically this request can be interfered
> +   * by other kind of requests as well, but that problem is going to be handled
> +   * on bug 1147776.
> +   */

Ditto.

@@ +988,5 @@
> +   * Try to write the characteristic value through a non-authenticated physical
> +   * link at the beginning. If the operation is failed due to insufficient
> +   * authentication or insufficient encryption key size, we try to write it
> +   * through an authenticated physical link again.
> +   */

Ditto.

::: dom/bluetooth/bluetooth2/BluetoothGattCharacteristic.cpp
@@ +284,5 @@
> +
> +  BT_ENSURE_TRUE_REJECT((mProperties & GATT_CHAR_PROP_BIT_WRITE_NO_RESPONSE) ||
> +                        (mProperties & GATT_CHAR_PROP_BIT_WRITE) ||
> +                        (mProperties & GATT_CHAR_PROP_BIT_SIGNED_WRITE),
> +                        NS_ERROR_NOT_AVAILABLE);

Prefer to revise as following for clear indention.

  BT_ENSURE_TRUE_REJECT(mProperties &
                          (GATT_CHAR_PROP_BIT_WRITE_NO_RESPONSE ||
                           GATT_CHAR_PROP_BIT_WRITE ||
                           GATT_CHAR_PROP_BIT_SIGNED_WRITE),
                        NS_ERROR_NOT_AVAILABLE);
Attachment #8591543 - Flags: review?(btian) → review+

Comment 48

3 years ago
Comment on attachment 8583664 [details] [diff] [review]
bug1140952_03_gatt_read_write_descriptor_webapi.patch

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

r=me with comment addressed.

::: dom/bluetooth2/BluetoothGattDescriptor.cpp
@@ +58,5 @@
> +                                  JS::Rooted<JSObject*>* aValue) const
> +{
> +  MOZ_ASSERT(aValue);
> +
> +  if (mValue.Length()) {

I prefer to check |mValue.IsEmpty()| instead for clear meaning.

  *aValue = mValue.IsEmpty() ?
              nullptr :
              ArrayBuffer::Create(cx, mValue.Length(), mValue.Elements());

@@ +64,5 @@
> +  } else {
> +    *aValue = nullptr;
> +  }
> +
> +  return;

Remove the redundant return.
Attachment #8583664 - Flags: review?(btian) → review+

Comment 49

3 years ago
Comment on attachment 8591544 [details] [diff] [review]
bug1140952_04_gatt_read_write_descriptor_ipc.v4.patch

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

r=me with comment addressed.

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +1103,5 @@
> +   * is not respected properly. So only one request of reading a descriptor
> +   * value is handled at the same time. Theoretically this request can be
> +   * interfered by other kind of requests as well, but that problem is going to
> +   * be handled on bug 1147776.
> +   */

Revise per comment 47.

@@ +1117,5 @@
> +   * Try to read the descriptor value through a non-authenticated physical
> +   * link at the beginning. If the operation is failed due to insufficient
> +   * authentication or insufficient encryption key size, we try to read it
> +   * through an authenticated physical link again.
> +   */

Revise per comment 47.

@@ +1186,5 @@
> +   * is not respected properly. So only one request of writing a descriptor
> +   * value is handled at the same time. Theoretically this request can be
> +   * interfered by other kind of requests as well, but that problem is going to
> +   * be handled on bug 1147776.
> +   */

Revise per comment 47.

@@ +1198,5 @@
> +   * Try to write the descriptor value through a non-authenticated physical
> +   * link at the beginning. If the operation is failed due to insufficient
> +   * authentication or insufficient encryption key size, we try to write it
> +   * through an authenticated physical link again.
> +   */

Revise per comment 47.

::: dom/bluetooth/bluetooth2/BluetoothGattDescriptor.cpp
@@ +161,5 @@
> +  NS_ENSURE_TRUE(!aRv.Failed(), nullptr);
> +
> +  GattCharacteristicProperties properties;
> +  mCharacteristic->GetProperties(properties);
> +  BT_ENSURE_TRUE_REJECT(properties.mRead, NS_ERROR_NOT_AVAILABLE);

Store |properties.mRead| as member variable |mReadable| during construction, and check |mReadable| here directly to save query effort.

@@ +192,5 @@
> +  NS_ENSURE_TRUE(!aRv.Failed(), nullptr);
> +
> +  GattCharacteristicProperties properties;
> +  mCharacteristic->GetProperties(properties);
> +  BT_ENSURE_TRUE_REJECT(properties.mWrite, NS_ERROR_NOT_AVAILABLE);

Ditto. Store boolean |mWritable| during construction.
Attachment #8591544 - Flags: review?(btian) → review+

Comment 50

3 years ago
Comment on attachment 8591543 [details] [diff] [review]
bug1140952_02_gatt_read_write_characteristic_ipc.v4.patch

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

Some more comment...

::: dom/bluetooth/bluetooth2/BluetoothGattCharacteristic.cpp
@@ +253,5 @@
> +
> +  nsRefPtr<Promise> promise = Promise::Create(global, aRv);
> +  NS_ENSURE_TRUE(!aRv.Failed(), nullptr);
> +
> +  BT_ENSURE_TRUE_REJECT(mProperties & GATT_CHAR_PROP_BIT_READ,

You may remember read bit value as member variable boolean |mReadable| during construction to conform with BluetoothGattDescriptor.

@@ +283,5 @@
> +  NS_ENSURE_TRUE(!aRv.Failed(), nullptr);
> +
> +  BT_ENSURE_TRUE_REJECT((mProperties & GATT_CHAR_PROP_BIT_WRITE_NO_RESPONSE) ||
> +                        (mProperties & GATT_CHAR_PROP_BIT_WRITE) ||
> +                        (mProperties & GATT_CHAR_PROP_BIT_SIGNED_WRITE),

You may remember write bit value as member variable boolean |mWritable| during construction to conform with BluetoothGattDescriptor.
(Assignee)

Comment 51

3 years ago
> I prefer to check |mValue.IsEmpty()| instead for clear meaning.

Thanks Ben. |mValue.IsEmpty()| is much clearer. I'm going to modify indents according to the coding style guideline[1] if you don't mind.

  *aValue = mValue.IsEmpty()
            ? nullptr
            : ArrayBuffer::Create(cx, mValue.Length(), mValue.Elements());

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators

Comment 52

3 years ago
(In reply to Bruce Sun [:brsun] from comment #51)
> > I prefer to check |mValue.IsEmpty()| instead for clear meaning.
> 
> Thanks Ben. |mValue.IsEmpty()| is much clearer. I'm going to modify indents
> according to the coding style guideline[1] if you don't mind.
> 
>   *aValue = mValue.IsEmpty()
>             ? nullptr
>             : ArrayBuffer::Create(cx, mValue.Length(), mValue.Elements());
> 
> [1]
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Coding_Style#Operators

Of course not. Please go ahead.
(Assignee)

Comment 53

3 years ago
> @@ +192,5 @@
> > +  NS_ENSURE_TRUE(!aRv.Failed(), nullptr);
> > +
> > +  GattCharacteristicProperties properties;
> > +  mCharacteristic->GetProperties(properties);
> > +  BT_ENSURE_TRUE_REJECT(properties.mWrite, NS_ERROR_NOT_AVAILABLE);
> 
> Ditto. Store boolean |mWritable| during construction.

I found that it is not necessary to check the properties of the characteristic before read/write the value of the descriptor. I'll remove the checking and upload another patch again.
(Assignee)

Comment 54

3 years ago
> You may remember read bit value as member variable boolean |mReadable|
> during construction to conform with BluetoothGattDescriptor.
> 
> You may remember write bit value as member variable boolean |mWritable|
> during construction to conform with BluetoothGattDescriptor.

Hi Ben, I'm going to remove the checking of |mReadable| and |mWritable| from |BluetoothGattDescriptor| as comment 53. Do you still suggest to use |mReadable| and |mWritable| within |BluetoothGattCharacteristic|?
Flags: needinfo?(btian)

Comment 55

3 years ago
(In reply to Bruce Sun [:brsun] from comment #54) 
> Hi Ben, I'm going to remove the checking of |mReadable| and |mWritable| from
> |BluetoothGattDescriptor| as comment 53. Do you still suggest to use
> |mReadable| and |mWritable| within |BluetoothGattCharacteristic|?

I'm fine to keep original bitwise check as the query time is identical.
Flags: needinfo?(btian)
(Assignee)

Comment 56

3 years ago
Created attachment 8592600 [details] [diff] [review]
bug1140952_01_gatt_read_write_characteristic_webapi.v4.patch

Hi Blake,

Would you mind helping to review this patch regarding to WebIDL part?

This patch adds capabilities to read/write values of BluetoothGattCharacteristic. Users can check whether this characteristic can be read or written through corresponding values of GattCharacteristicProperties.

Two attributes ('value'[1] and 'properties'[2]) and two methods ('readValue'[3] and 'writeValue'[4]) are added into BluetoothGattCharacteristic.webidl accordingly.

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGattCharacteristic#value
[2] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGattCharacteristic#GattCharacteristicProperties
[3] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGattCharacteristic#readValue.28.29
[4] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGattCharacteristic#writeValue.28ArrayBuffer_value.29
Attachment #8589506 - Attachment is obsolete: true
Attachment #8592600 - Flags: review?(mrbkap)
(Assignee)

Comment 57

3 years ago
Created attachment 8592601 [details] [diff] [review]
bug1140952_03_gatt_read_write_descriptor_webapi.v3.patch

This patch adds capabilities to read/write values of BluetoothGattDescriptor. Comparing to BluetoothGattCharacteristic, there is no need to check any properties before reading or writing values.

One attribute ('value'[1]) and two methods ('readValue'[2] and 'writeValue'[3]) are added into BluetoothGattDescriptor.webidl accordingly.

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGattDescriptor#value
[2] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGattDescriptor#readValue.28.29
[3] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGattDescriptor#writeValue.28ArrayBuffer_value.29
Attachment #8583664 - Attachment is obsolete: true
Attachment #8592601 - Flags: review?(mrbkap)
Comment on attachment 8592600 [details] [diff] [review]
bug1140952_01_gatt_read_write_characteristic_webapi.v4.patch

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

r=me with my comment addressed.

::: dom/bluetooth/bluetooth2/BluetoothGattCharacteristic.h
@@ +59,5 @@
>    {
>      return mCharId.mInstanceId;
>    }
>  
> +  void GetValue(JSContext* cx, JS::Rooted<JSObject*>* aValue) const;

The second parameter should be a JS::MutableHandle<JSObject*> instead of a pointer to a JS::Rooted (see js/public/RootingAPI.h for more info).
Attachment #8592600 - Flags: review?(mrbkap) → review+
Comment on attachment 8592601 [details] [diff] [review]
bug1140952_03_gatt_read_write_descriptor_webapi.v3.patch

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

r=me with my comment addressed.

::: dom/bluetooth/bluetooth2/BluetoothGattDescriptor.h
@@ +41,5 @@
>    {
>      aUuidStr = mUuidStr;
>    }
>  
> +  void GetValue(JSContext* cx, JS::Rooted<JSObject*>* aValue) const;

Ditto here (JS::MutableHandle<JSObject*>).
Attachment #8592601 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 60

3 years ago
Created attachment 8593301 [details] [diff] [review]
bug1140952_01_gatt_read_write_characteristic_webapi.commit.patch

Address comment 58.
Attachment #8592600 - Attachment is obsolete: true
Attachment #8593301 - Flags: review+
(Assignee)

Comment 61

3 years ago
Created attachment 8593302 [details] [diff] [review]
bug1140952_02_gatt_read_write_characteristic_ipc.commit.patch
Attachment #8591543 - Attachment is obsolete: true
Attachment #8593302 - Flags: review+
(Assignee)

Comment 62

3 years ago
Created attachment 8593303 [details] [diff] [review]
bug1140952_03_gatt_read_write_descriptor_webapi.commit.patch

Address comment 59.
Attachment #8592601 - Attachment is obsolete: true
Attachment #8593303 - Flags: review+
(Assignee)

Comment 63

3 years ago
Created attachment 8593305 [details] [diff] [review]
bug1140952_04_gatt_read_write_descriptor_ipc.commit.patch
Attachment #8591544 - Attachment is obsolete: true
Attachment #8593305 - Flags: review+
(Assignee)

Updated

3 years ago
Blocks: 1152653
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.