Closed Bug 1170071 Opened 9 years ago Closed 9 years ago

[Bluetooth2] Implement GATT server support in daemon backend

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

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

Tracking

(firefox42 fixed)

Status:
RESOLVED FIXED
Milestone:
FxOS-S2 (10Jul)
Tracking Flags:
Tracking Status
firefox42 --- fixed

People

(Reporter: yrliou, Assigned: yrliou)

References

Details

(Whiteboard: [webbt-api])

Attachments

(4 files, 6 obsolete files)

[Final] Bug 1170071 - Patch1: Revise BluetoothGattInterface, result handler, and interface helper for daemon support of gatt server. r=shuang
13.33 KB, patch
Details | Diff | Splinter Review
[Final] Bug 1170071 - Patch2: Add helpers for Bluetooth daemon GATT server support. r=shuang
2.36 KB, patch
Details | Diff | Splinter Review
[Final] Bug 1170071 - Patch3: Add gatt server support to GATT module for Bluetooth daemon. r=shuang
57.64 KB, patch
Details | Diff | Splinter Review
[Final] Bug 1170071 - Patch4: Add Gatt server interfaces for Bluetooth daemon. r=shuang
13.93 KB, patch
Details | Diff | Splinter Review 
This bug is to trace the GATT server support work in gecko using daemon backend.
Depends on: 1161991
Blocks: 933358
Attachment #8630350 - Flags: review?(shuang)
Attachment #8630351 - Flags: review?(shuang)
Attachment #8630352 - Flags: review?(shuang)
Attachment #8630353 - Flags: review?(shuang)
Hi Shuang,

This bug is for adding gatt server support in daemon backend, could you help to review my patches?

Thanks,
Jocelyn
(In reply to Jocelyn Liu [:jocelyn] [:joliu] [PTO from 7/9~7/24] from comment #5)
> Hi Shuang,
> 
Sorry, should be Shawn. :/

> This bug is for adding gatt server support in daemon backend, could you help
> to review my patches?
> 
> Thanks,
> Jocelyn
Comment on attachment 8630350 [details] [diff] [review]
Bug 1170071 - Patch1: Revise BluetoothGattInterface, result handler, and interface helper for daemon support of gatt server.

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

Looks good, but just want to make sure six-parameter is necessary.

::: dom/bluetooth/BluetoothInterfaceHelpers.h
@@ +700,5 @@
> +          typename Tin1, typename Tin2, typename Tin3,
> +          typename Tin4, typename Tin5, typename Tin6,
> +          typename Arg1=Tin1, typename Arg2=Tin2, typename Arg3=Tin3,
> +          typename Arg4=Tin4, typename Arg5=Tin5, typename Arg6=Tin6>
> +class BluetoothNotificationRunnable6 : public nsRunnable

Did I miss something? I only see 9 parameters version used instead of 6 parameters in this patch, is this necessary?
Comment on attachment 8630351 [details] [diff] [review]
Bug 1170071 - Patch2: Add helpers for Bluetooth daemon GATT server support.

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

::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp
@@ +34,5 @@
> +Convert(bool aIn, int32_t& aOut)
> +{
> +  static const bool sValue[] = {
> +    CONVERT(false, 0x00),
> +    CONVERT(true, 0x01)

Out of curiosity, this is been used to convert what value?
Attachment #8630351 - Flags: review?(shuang) → review+
Comment on attachment 8630352 [details] [diff] [review]
Bug 1170071 - Patch3: Add gatt server support to GATT module for Bluetooth daemon.

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

I will review for the rest of detail (such as PackPDU/UnPackPUD). I just want to ask some question first.

::: dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.cpp
@@ +82,5 @@
>    MOZ_ASSERT(NS_IsMainThread());
>  
>    nsAutoPtr<DaemonSocketPDU> pdu(
>      new DaemonSocketPDU(SERVICE_ID, OPCODE_CLIENT_REGISTER,
> +                        16)); // Service UUID

I prefer to have coding style tweaks  in an another patch.

@@ +1477,5 @@
> +    const DaemonSocketPDUHeader&,
> +    DaemonSocketPDU&,
> +    BluetoothGattServerResultHandler*) = {
> +    INIT_ARRAY_AT(0, nullptr),
> +    INIT_ARRAY_AT(OPCODE_CLIENT_REGISTER, nullptr),

It sounds a bit strange for me, do we need to put client opcode here (since the parameter is BluetoothGattServerResultHandler)? Maybe I did not be aware of any restriction?

@@ +2277,5 @@
> +    INIT_ARRAY_AT(26, &BluetoothDaemonGattModule::ServerServiceDeletedNtf),
> +    INIT_ARRAY_AT(27, &BluetoothDaemonGattModule::ServerRequestReadNtf),
> +    INIT_ARRAY_AT(28, &BluetoothDaemonGattModule::ServerRequestWriteNtf),
> +    INIT_ARRAY_AT(29, &BluetoothDaemonGattModule::ServerRequestExecuteWriteNtf),
> +    INIT_ARRAY_AT(30, &BluetoothDaemonGattModule::ServerResponseConfirmationNtf)

I checked bt_gatt_server.h, it seems there are three more callback functions? Is it complete implementation?

...
    indication_sent_callback        indication_sent_cb;
    congestion_callback             congestion_cb;
    mtu_changed_callback            mtu_changed_cb;
} btgatt_server_callbacks_t;
(In reply to Shawn Huang [:shawnjohnjr] from comment #7)
> Comment on attachment 8630350 [details] [diff] [review]
> Bug 1170071 - Patch1: Revise BluetoothGattInterface, result handler, and
> interface helper for daemon support of gatt server.
> 
> Review of attachment 8630350 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, but just want to make sure six-parameter is necessary.
> 
> ::: dom/bluetooth/BluetoothInterfaceHelpers.h
> @@ +700,5 @@
> > +          typename Tin1, typename Tin2, typename Tin3,
> > +          typename Tin4, typename Tin5, typename Tin6,
> > +          typename Arg1=Tin1, typename Arg2=Tin2, typename Arg3=Tin3,
> > +          typename Arg4=Tin4, typename Arg5=Tin5, typename Arg6=Tin6>
> > +class BluetoothNotificationRunnable6 : public nsRunnable
> 
> Did I miss something? I only see 9 parameters version used instead of 6
> parameters in this patch, is this necessary?

Yes, it's used in ServerRequestReadNotification, which is in patch3.
(In reply to Shawn Huang [:shawnjohnjr] from comment #9)
> Comment on attachment 8630352 [details] [diff] [review]
> Bug 1170071 - Patch3: Add gatt server support to GATT module for Bluetooth
> daemon.
> 
> Review of attachment 8630352 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I will review for the rest of detail (such as PackPDU/UnPackPUD). I just
> want to ask some question first.
> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.cpp
> @@ +82,5 @@
> >    MOZ_ASSERT(NS_IsMainThread());
> >  
> >    nsAutoPtr<DaemonSocketPDU> pdu(
> >      new DaemonSocketPDU(SERVICE_ID, OPCODE_CLIENT_REGISTER,
> > +                        16)); // Service UUID
> 
> I prefer to have coding style tweaks  in an another patch.
> 
Sure. Is it OK to include it in this bug?
> @@ +1477,5 @@
> > +    const DaemonSocketPDUHeader&,
> > +    DaemonSocketPDU&,
> > +    BluetoothGattServerResultHandler*) = {
> > +    INIT_ARRAY_AT(0, nullptr),
> > +    INIT_ARRAY_AT(OPCODE_CLIENT_REGISTER, nullptr),
> 
> It sounds a bit strange for me, do we need to put client opcode here (since
> the parameter is BluetoothGattServerResultHandler)? Maybe I did not be aware
> of any restriction?
> 

I will encounter build error "sorry, unimplemented: non-trivial designated initializers not supported" if I didn't start from 0 or skip an index.
In long term, I think we should combine result handler, interface, ... of gatt server and client into a single one for gatt as bluez HAL does.
But I would like to do it after we remove HAL, that would be much easier when doing a refactor.
I could open a bug for that and write some comments including the bug number near these lines, does this sounds OK to you?

> @@ +2277,5 @@
> > +    INIT_ARRAY_AT(26, &BluetoothDaemonGattModule::ServerServiceDeletedNtf),
> > +    INIT_ARRAY_AT(27, &BluetoothDaemonGattModule::ServerRequestReadNtf),
> > +    INIT_ARRAY_AT(28, &BluetoothDaemonGattModule::ServerRequestWriteNtf),
> > +    INIT_ARRAY_AT(29, &BluetoothDaemonGattModule::ServerRequestExecuteWriteNtf),
> > +    INIT_ARRAY_AT(30, &BluetoothDaemonGattModule::ServerResponseConfirmationNtf)
> 
> I checked bt_gatt_server.h, it seems there are three more callback
> functions? Is it complete implementation?
> 
> ...
>     indication_sent_callback        indication_sent_cb;
>     congestion_callback             congestion_cb;
>     mtu_changed_callback            mtu_changed_cb;
> } btgatt_server_callbacks_t;

These functions are added in L, I didn't cover L support at all but only left TODO comment in this version.
Two main reasons here,
1) Those functions don't have clear use cases at this moment.
2) Sequence of opcodes in bluez HAL are client-operations-before-L, server-operations-before-L, client-operations-added-in-L, then server-operations-added-in-L.
   Now we use separate result handlers for client and server, code will be really messy if we add L support right now.
   For example, https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.cpp#944, in order to support L, we need to add opcodes of all server-operations-before-L, then we could add client-operations-added-in-L.
Comment on attachment 8630350 [details] [diff] [review]
Bug 1170071 - Patch1: Revise BluetoothGattInterface, result handler, and interface helper for daemon support of gatt server.

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

Per comment 10, this patch looks good to me.
Attachment #8630350 - Flags: review?(shuang) → review+
(In reply to Jocelyn Liu [:jocelyn] [:joliu] [PTO from 7/9~7/24] from comment #11)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #9)
> > Comment on attachment 8630352 [details] [diff] [review]
> > Bug 1170071 - Patch3: Add gatt server support to GATT module for Bluetooth
> > daemon.
> > 
> > Review of attachment 8630352 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > It sounds a bit strange for me, do we need to put client opcode here (since
> > the parameter is BluetoothGattServerResultHandler)? Maybe I did not be aware
> > of any restriction?
> > 
> 
> I will encounter build error "sorry, unimplemented: non-trivial designated
> initializers not supported" if I didn't start from 0 or skip an index.
> In long term, I think we should combine result handler, interface, ... of
> gatt server and client into a single one for gatt as bluez HAL does.
> But I would like to do it after we remove HAL, that would be much easier
> when doing a refactor.
> I could open a bug for that and write some comments including the bug number
> near these lines, does this sounds OK to you?
That sounds good to me.
> These functions are added in L, I didn't cover L support at all but only
> left TODO comment in this version.
> Two main reasons here,
> 1) Those functions don't have clear use cases at this moment.
> 2) Sequence of opcodes in bluez HAL are client-operations-before-L,
> server-operations-before-L, client-operations-added-in-L, then
> server-operations-added-in-L.
>    Now we use separate result handlers for client and server, code will be
> really messy if we add L support right now.
>    For example,
> https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/
> BluetoothDaemonGattInterface.cpp#944, in order to support L, we need to add
> opcodes of all server-operations-before-L, then we could add
> client-operations-added-in-L.
I see. Then I probably need to do it better when we support L.
(In reply to Shawn Huang [:shawnjohnjr] from comment #8)
> Comment on attachment 8630351 [details] [diff] [review]
> Bug 1170071 - Patch2: Add helpers for Bluetooth daemon GATT server support.
> 
> Review of attachment 8630351 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp
> @@ +34,5 @@
> > +Convert(bool aIn, int32_t& aOut)
> > +{
> > +  static const bool sValue[] = {
> > +    CONVERT(false, 0x00),
> > +    CONVERT(true, 0x01)
> 
> Out of curiosity, this is been used to convert what value?

For PackConversion<bool, int32_t>(aConfirm) call in ServerSendIndicationCmd in patch3.
Comment on attachment 8630352 [details] [diff] [review]
Bug 1170071 - Patch3: Add gatt server support to GATT module for Bluetooth daemon.

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

One question regarding Server Send Response. Will do final check once question get clarified.

::: dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.cpp
@@ +716,5 @@
> +  nsAutoPtr<DaemonSocketPDU> pdu(
> +    new DaemonSocketPDU(SERVICE_ID, OPCODE_SERVER_UNREGISTER,
> +                        4)); // Server Interface
> +
> +  nsresult rv = PackPDU(PackConversion<int, int32_t>(aServerIf), *pdu);

Good catch! I saw |ClientUnregisterCmd| still stays with int instead of explicitly convert to int32_t. :p

@@ +769,5 @@
> +  nsAutoPtr<DaemonSocketPDU> pdu(
> +    new DaemonSocketPDU(SERVICE_ID, OPCODE_SERVER_DISCONNECT_PERIPHERAL,
> +                        4 + // Server Interface
> +                        6 + // Remote Address
> +                        4)); // Connection Id

Good catch! It looks like hal-ipc-api.txt is wrong again.

@@ +1014,5 @@
> +    new DaemonSocketPDU(SERVICE_ID, OPCODE_SERVER_SEND_RESPONSE,
> +                        0));
> +
> +  nsresult rv = PackPDU(PackConversion<int, int32_t>(aServerIf),
> +                        PackConversion<int, int32_t>(aTransId),

It looks like more parameters than we think. Can you clarify it?

	Opcode 0x23 - Server Send Response command/response

		Command parameters: Connection ID (4 octets)
		                    Transaction ID (4 octets)
		                    Handle (2 octets)
		                    Offset (2 octets)
		                    Auth Request (1 octect)
		                    Status (4 octets)
		                    GATT Response (4 octets)
		Response parameters: <none>
(In reply to Shawn Huang [:shawnjohnjr] from comment #15)
> Comment on attachment 8630352 [details] [diff] [review]
> Bug 1170071 - Patch3: Add gatt server support to GATT module for Bluetooth
> daemon.
> 
> Review of attachment 8630352 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> One question regarding Server Send Response. Will do final check once
> question get clarified.
> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.cpp
> @@ +716,5 @@
> > +  nsAutoPtr<DaemonSocketPDU> pdu(
> > +    new DaemonSocketPDU(SERVICE_ID, OPCODE_SERVER_UNREGISTER,
> > +                        4)); // Server Interface
> > +
> > +  nsresult rv = PackPDU(PackConversion<int, int32_t>(aServerIf), *pdu);
> 
> Good catch! I saw |ClientUnregisterCmd| still stays with int instead of
> explicitly convert to int32_t. :p
> 
> @@ +769,5 @@
> > +  nsAutoPtr<DaemonSocketPDU> pdu(
> > +    new DaemonSocketPDU(SERVICE_ID, OPCODE_SERVER_DISCONNECT_PERIPHERAL,
> > +                        4 + // Server Interface
> > +                        6 + // Remote Address
> > +                        4)); // Connection Id
> 
> Good catch! It looks like hal-ipc-api.txt is wrong again.
> 
> @@ +1014,5 @@
> > +    new DaemonSocketPDU(SERVICE_ID, OPCODE_SERVER_SEND_RESPONSE,
> > +                        0));
> > +
> > +  nsresult rv = PackPDU(PackConversion<int, int32_t>(aServerIf),
> > +                        PackConversion<int, int32_t>(aTransId),
> 
> It looks like more parameters than we think. Can you clarify it?
> 
> 	Opcode 0x23 - Server Send Response command/response
> 
> 		Command parameters: Connection ID (4 octets)
> 		                    Transaction ID (4 octets)
> 		                    Handle (2 octets)
> 		                    Offset (2 octets)
> 		                    Auth Request (1 octect)
> 		                    Status (4 octets)
> 		                    GATT Response (4 octets)
> 		Response parameters: <none>

You were right, unpacking part in this function is wrong, I will fix it and upload a new patch3, it should follow the structure defined in https://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/hal-msg.h#n1045.
Comment on attachment 8630352 [details] [diff] [review]
Bug 1170071 - Patch3: Add gatt server support to GATT module for Bluetooth daemon.

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

Per the previous comment, we should fix SendResponse wrong parameters.

::: dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.cpp
@@ +1558,5 @@
>      nsRefPtr<BluetoothGattClientResultHandler> res =
>        already_AddRefed<BluetoothGattClientResultHandler>(
>          static_cast<BluetoothGattClientResultHandler*>(aUserData));
>      (this->*(HandleClientRsp[aHeader.mOpcode]))(aHeader, aPDU, res);
> +  } else { // isInGattServerArray

could it be |} else if (isInGattServerArray) {|? Is it possible that both isInGattClientArray and isInGattServerArray all false but isInGattArray equals to true.
Attachment #8630352 - Flags: review?(shuang) → review-
- fix ServerSendResponseCmd

I'll add the comment I mentioned in Comment 11 in the next/final version.
Attachment #8630352 - Attachment is obsolete: true
Attachment #8630942 - Flags: review?(shuang)
- add GetBluetoothGattServerInterface
Attachment #8630353 - Attachment is obsolete: true
Attachment #8630353 - Flags: review?(shuang)
Attachment #8630943 - Flags: review?(shuang)
Comment on attachment 8630943 [details] [diff] [review]
Bug 1170071 - Patch4(v2): Add Gatt server interfaces for Bluetooth daemon.

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

It looks good to me. But I wonder you probably need to call sBluetoothGattClientInterface->GetBluetoothGattServerInterface in BluetoothGattManager::InitGattInterface?
Attachment #8630943 - Flags: review?(shuang) → review+
Comment on attachment 8630942 [details] [diff] [review]
Bug 1170071 - Patch3(v2): Add gatt server support to GATT module for Bluetooth daemon.

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

Ok, after checking all these parameters again with hal-api, things look good. Please add comments per Comment 11 and open follow-up bugs.
Attachment #8630942 - Flags: review?(shuang) → review+
- add a comment before HandleServerRsp array
- filed refactor bug 1181512
Attachment #8630942 - Attachment is obsolete: true
- add server interface initialization to GattManager
Attachment #8630943 - Attachment is obsolete: true
try looks fine.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: