1161991 - Gatt server neutral-backend interface

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Comment 1

[Bruce Sun [:brsun]]

Attachment: bug1161991_gatt_server_hal_interface.patch

Comment 2

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

Comment on attachment 8607431 [details] [diff] [review]
bug1161991_gatt_server_hal_interface.patch

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

r=me after comments addressed or answered, thanks.

::: dom/bluetooth/BluetoothInterface.h
@@ +641,5 @@
> +
> +  virtual void
> +  CharacteristicAddedNotification(BluetoothGattStatus aStatus,
> +                                  int aServerIf,
> +                                  const BluetoothUuid& aCharId,

nit: suggest to use aCharUuid.

@@ +649,5 @@
> +
> +  virtual void
> +  DescriptorAddedNotification(BluetoothGattStatus aStatus,
> +                              int aServerIf,
> +                              const BluetoothUuid& aCharId,

aDescUuid

::: dom/bluetooth/bluedroid/BluetoothHALHelpers.h
@@ +823,5 @@
> +    CONVERT(GATT_STATUS_UNSUPPORTED_GROUP_TYPE, 0x0010),
> +    CONVERT(GATT_STATUS_INSUFFICIENT_RESOURCES, 0x0011)
> +  };
> +  if (static_cast<uint32_t>(aIn) >= MOZ_ARRAY_LENGTH(sGattStatus)) {
> +    aOut = -1;

Why do we return NS_OK in this case?
How would a remote gatt client act if it receives status code as -1?

@@ -890,5 @@
>  Convert(const btgatt_write_params_t& aIn, BluetoothGattWriteParam& aOut);
>  
>  nsresult
>  Convert(const btgatt_notify_params_t& aIn, BluetoothGattNotifyParam& aOut);
> -#endif // ANDROID_VERSION >= 19

why remove this line?

@@ -892,5 @@
>  nsresult
>  Convert(const btgatt_notify_params_t& aIn, BluetoothGattNotifyParam& aOut);
> -#endif // ANDROID_VERSION >= 19
> -
> -#if ANDROID_VERSION >= 21

why is this removed instead of moving to after transport convert function?

Comment 3

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

Comment on attachment 8607431 [details] [diff] [review]
bug1161991_gatt_server_hal_interface.patch

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

::: dom/bluetooth/bluedroid/BluetoothGattHALInterface.cpp
@@ +1601,5 @@
> +#if ANDROID_VERSION >= 19
> +  int response_status;
> +  if (NS_SUCCEEDED(Convert(aStatus, response_status))) {
> +    status = mInterface->send_response(aConnId, aTransId, response_status,
> +                                       nullptr); // TODO

why is this unimplemented?
Also, bluedroid and bluez HAL both seems to use the same structure for read/write responses, what's the reason to separate them in BluetoothCommon.h?

Comment 4

[Bruce Sun [:brsun]]

Comment on attachment 8607431 [details] [diff] [review]
bug1161991_gatt_server_hal_interface.patch

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

::: dom/bluetooth/bluedroid/BluetoothGattHALInterface.cpp
@@ +1601,5 @@
> +#if ANDROID_VERSION >= 19
> +  int response_status;
> +  if (NS_SUCCEEDED(Convert(aStatus, response_status))) {
> +    status = mInterface->send_response(aConnId, aTransId, response_status,
> +                                       nullptr); // TODO

|btgatt_response_t|[1] and |tGATTS_RSP|[2] in bluedroid are defined as unions. Although |btgatt_server_interface_t.send_response()|[3] use |btgatt_response_t *| for both read responses and write responses, they use different structures by design from the comment of |tGATTS_RSP|[2]. Otherwise there seems no other reasons to define |btgatt_response_t| as an union in |hardware/bt_gatt_server.h| (or maybe I miss something important).

In practical, we might be able to use |btgatt_value_t| and |tGATT_VALUE| only for both read and write responses because |btif_to_bta_response()| in AOSP[4] only handles |btgatt_value_t| and |tGATT_VALUE| parts. But it might differ from one implementation to another.

In order to differentiate between read responses and write responses in Gecko, |BluetoothGattServer.sendResponse()|[5] probably has to be separated into |sendReadResponse()| and |sendWriteResponse()| to have a proper assignment for |btgatt_response_t| union.

Will you suggest to use only |btgatt_value_t| of |btgatt_response_t| for both read and write cases?

[1] http://androidxref.com/5.1.0_r1/xref/hardware/libhardware/include/hardware/bt_gatt_server.h#38
[2] http://androidxref.com/5.1.0_r1/xref/external/bluetooth/bluedroid/stack/include/gatt_api.h#322
[3] http://androidxref.com/5.1.0_r1/xref/hardware/libhardware/include/hardware/bt_gatt_server.h#189
[4] http://androidxref.com/5.1.0_r1/xref/external/bluetooth/bluedroid/btif/src/btif_gatt_util.c#123
[5] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGattServer#sendResponse.28DOMString_address.2C_int_status.2C_int_requestId.2C_ArrayBuffer_value.29

::: dom/bluetooth/bluedroid/BluetoothHALHelpers.h
@@ +823,5 @@
> +    CONVERT(GATT_STATUS_UNSUPPORTED_GROUP_TYPE, 0x0010),
> +    CONVERT(GATT_STATUS_INSUFFICIENT_RESOURCES, 0x0011)
> +  };
> +  if (static_cast<uint32_t>(aIn) >= MOZ_ARRAY_LENGTH(sGattStatus)) {
> +    aOut = -1;

I will add |return NS_ERROR_ILLEGAL_VALUE;| here.

@@ -890,5 @@
>  Convert(const btgatt_write_params_t& aIn, BluetoothGattWriteParam& aOut);
>  
>  nsresult
>  Convert(const btgatt_notify_params_t& aIn, BluetoothGattNotifyParam& aOut);
> -#endif // ANDROID_VERSION >= 19

This line is not removed. |Convert(BluetoothTransport aIn, int& aOut)| is moved into the scope of |ANDROID_VERSION >= 19| instead.

@@ -892,5 @@
>  nsresult
>  Convert(const btgatt_notify_params_t& aIn, BluetoothGattNotifyParam& aOut);
> -#endif // ANDROID_VERSION >= 19
> -
> -#if ANDROID_VERSION >= 21

Well...It depends how the diff tool represents this patch. |Convert(BluetoothTransport aIn, int& aOut)| is moved into the scope of |ANDROID_VERSION >= 19| instead.

Comment 5

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

Hi Bruce,

According to bluez HAL API[1], read/write responses seem to use the same structure.
Personally I would prefer to follow this HAL API since we will switch to bluetoothd soon.
There're some duplicate parameters in the documentation though, I suppose that was a mistake based on what I saw in hal-msg.h[2].

On the other hand, android also passes btgatt_value_t for both read and write requests.[3]

[1] https://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/hal-ipc-api.txt#n1816 
[2] https://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/hal-msg.h#n1045
[3] http://androidxref.com/5.1.0_r1/xref/packages/apps/Bluetooth/jni/com_android_bluetooth_gatt.cpp#1675

(In reply to Bruce Sun [:brsun] from comment #4)
> Comment on attachment 8607431 [details] [diff] [review]
> bug1161991_gatt_server_hal_interface.patch
> 
> Review of attachment 8607431 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothGattHALInterface.cpp
> @@ +1601,5 @@
> > +#if ANDROID_VERSION >= 19
> > +  int response_status;
> > +  if (NS_SUCCEEDED(Convert(aStatus, response_status))) {
> > +    status = mInterface->send_response(aConnId, aTransId, response_status,
> > +                                       nullptr); // TODO
> 
> |btgatt_response_t|[1] and |tGATTS_RSP|[2] in bluedroid are defined as
> unions. Although |btgatt_server_interface_t.send_response()|[3] use
> |btgatt_response_t *| for both read responses and write responses, they use
> different structures by design from the comment of |tGATTS_RSP|[2].
> Otherwise there seems no other reasons to define |btgatt_response_t| as an
> union in |hardware/bt_gatt_server.h| (or maybe I miss something important).
> 
> In practical, we might be able to use |btgatt_value_t| and |tGATT_VALUE|
> only for both read and write responses because |btif_to_bta_response()| in
> AOSP[4] only handles |btgatt_value_t| and |tGATT_VALUE| parts. But it might
> differ from one implementation to another.
> 
> In order to differentiate between read responses and write responses in
> Gecko, |BluetoothGattServer.sendResponse()|[5] probably has to be separated
> into |sendReadResponse()| and |sendWriteResponse()| to have a proper
> assignment for |btgatt_response_t| union.
> 
> Will you suggest to use only |btgatt_value_t| of |btgatt_response_t| for
> both read and write cases?
> 
> [1]
> http://androidxref.com/5.1.0_r1/xref/hardware/libhardware/include/hardware/
> bt_gatt_server.h#38
> [2]
> http://androidxref.com/5.1.0_r1/xref/external/bluetooth/bluedroid/stack/
> include/gatt_api.h#322
> [3]
> http://androidxref.com/5.1.0_r1/xref/hardware/libhardware/include/hardware/
> bt_gatt_server.h#189
> [4]
> http://androidxref.com/5.1.0_r1/xref/external/bluetooth/bluedroid/btif/src/
> btif_gatt_util.c#123
> [5]
> https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/
> BluetoothGattServer#sendResponse.28DOMString_address.2C_int_status.
> 2C_int_requestId.2C_ArrayBuffer_value.29
> 
> ::: dom/bluetooth/bluedroid/BluetoothHALHelpers.h
> @@ +823,5 @@
> > +    CONVERT(GATT_STATUS_UNSUPPORTED_GROUP_TYPE, 0x0010),
> > +    CONVERT(GATT_STATUS_INSUFFICIENT_RESOURCES, 0x0011)
> > +  };
> > +  if (static_cast<uint32_t>(aIn) >= MOZ_ARRAY_LENGTH(sGattStatus)) {
> > +    aOut = -1;
> 
> I will add |return NS_ERROR_ILLEGAL_VALUE;| here.
> 
> @@ -890,5 @@
> >  Convert(const btgatt_write_params_t& aIn, BluetoothGattWriteParam& aOut);
> >  
> >  nsresult
> >  Convert(const btgatt_notify_params_t& aIn, BluetoothGattNotifyParam& aOut);
> > -#endif // ANDROID_VERSION >= 19
> 
> This line is not removed. |Convert(BluetoothTransport aIn, int& aOut)| is
> moved into the scope of |ANDROID_VERSION >= 19| instead.
> 
> @@ -892,5 @@
> >  nsresult
> >  Convert(const btgatt_notify_params_t& aIn, BluetoothGattNotifyParam& aOut);
> > -#endif // ANDROID_VERSION >= 19
> > -
> > -#if ANDROID_VERSION >= 21
> 
> Well...It depends how the diff tool represents this patch.
> |Convert(BluetoothTransport aIn, int& aOut)| is moved into the scope of
> |ANDROID_VERSION >= 19| instead.

Got it, just checked the raw patch, it looks fine.

Comment 6

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

Also, from your previous comment, "But it might differ from one implementation to another."
That's also the main reason I want to follow the HAL API using by bluetoothd instead of following bluedroid structures.

Comment 7

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

Comment on attachment 8607431 [details] [diff] [review]
bug1161991_gatt_server_hal_interface.patch

Please see comment 5 and comment 6 for my feedback, thanks!

Comment 8

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Just a word of warning: HAL is obsolete and will be removed after GATT is ready. You should re-consider spending time on this bug. We'd basically land the patch and remove it immediately afterwards.

Comment 9

[Bruce Sun [:brsun]]

Attachment: bug1161991_gatt_server_hal_interface.v2.patch

Address comment 2, comment 5, comment 6.

Comment 10

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

Comment on attachment 8615143 [details] [diff] [review]
bug1161991_gatt_server_hal_interface.v2.patch

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

Sorry for my delay here, please see my comments below.
Thanks for your effort, r=me after comments addressed.

::: dom/bluetooth/BluetoothCommon.h
@@ +705,5 @@
> +/*
> + * BluetoothGattAttrPerm is used to store a bit mask value which contains
> + * each corresponding bit value of each BluetoothGattAttrPermBit.
> + */
> +typedef uint16_t BluetoothGattAttrPerm;

Could we use int32_t for this?
int in http://androidxref.com/5.1.0_r1/xref/hardware/libhardware/include/hardware/bt_gatt_server.h#171
int32_t in https://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/hal-msg.h#n1009

@@ +786,5 @@
> +  uint8_t mValue[BLUETOOTH_GATT_MAX_ATTR_LEN];
> +  uint16_t mHandle;
> +  uint16_t mOffset;
> +  uint16_t mLength;
> +  BluetoothGattAuthReq mAuthReq;

Suggest to put mValue after mAuthReq.

::: dom/bluetooth/bluedroid/BluetoothHALHelpers.cpp
@@ +371,5 @@
> +nsresult
> +Convert(const BluetoothGattResponse& aIn, btgatt_response_t& aOut)
> +{
> +  // we assume the format of read response can be handled correctly by the
> +  // HAL backend for both read response and write response

I think we can revise this comment to describe that only the read response format is used in bluedroid.
http://androidxref.com/5.1.0_r1/xref/external/bluetooth/bluedroid/btif/src/btif_gatt_server.c#501
http://androidxref.com/5.1.0_r1/xref/external/bluetooth/bluedroid/btif/src/btif_gatt_util.c#123

Comment 11

[Bruce Sun [:brsun]]

Hi Jocelyn,

Thanks for your efforts!

> ::: dom/bluetooth/BluetoothCommon.h
> @@ +705,5 @@
> > +/*
> > + * BluetoothGattAttrPerm is used to store a bit mask value which contains
> > + * each corresponding bit value of each BluetoothGattAttrPermBit.
> > + */
> > +typedef uint16_t BluetoothGattAttrPerm;
> 
> Could we use int32_t for this?
> int in
> http://androidxref.com/5.1.0_r1/xref/hardware/libhardware/include/hardware/
> bt_gatt_server.h#171
> int32_t in
> https://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/hal-msg.h#n1009
> 

Since this type is used for bit-masking purposes, using an unsigned integer type as the native type of it might be more intuitive. What do you think if |uint32_t| is used in this case?

Comment 12

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

Hi Bruce,

Yes, unsigned type here seems reasonable and intuitive, but follow backend types also seems intuitive to me when we pack/unpack things.
This bit is actually unused, so I'm OK for both types.

Thanks,
Jocelyn

Comment 13

[Bruce Sun [:brsun]]

Attachment: bug1161991_gatt_server_hal_interface.commit.patch

Address comment 12.

Comment 14

[Bruce Sun [:brsun]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=47994d3b60e5

Comment 15

[Pulsebot]

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

Comment 16

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=a63ae8b44038

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a63ae8b44038