Closed Bug 1073548 Opened 10 years ago Closed 10 years ago

[Bluetooth] Implement support for Bluetooth daemon and basic interfaces

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S8 (7Nov)

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(7 files, 23 obsolete files)

1.03 KB, patch
Details | Diff | Splinter Review
2.87 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
18.13 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
3.69 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
56.25 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
73.60 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
28.74 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
Gecko needs support for the Bluetooth daemon. This includes

  - Build-system changes,
  - Socket IPC code,
  - Bluetooth PDUs, and
  - a Protocol parser.

The first version should implement support for the module handling, Core and Socket interfaces. These interfaces are required to be available. The interfaces for additional services can be added by later patches.
FYI, I've been working on this issue for several days and have some WIP patches. I hope to have a beta version until mid-October. The remaining services should be easy to add afterwards.
Attached patch [WIP] 09_fixes.patch (obsolete) — Splinter Review
Here are some WIP patches for Gecko. The patches

  - add socket IPC for communicating with the Bluetooth daemon;
  - add a basic protocol support;
  - add support for Setup, Core and Socket modules; and
  - allow switching on BT on a Nexus 4 without crashing Gecko. ;)

These patches still require a lot of debugging and cleanups. The corresponding daemon is available at 

  https://github.com/tdz/bluetoothd
Depends on: 1083092
Depends on: 1083708
See Also: → 1084257
Depends on: 1084342
Attachment #8503168 - Attachment is obsolete: true
Attachment #8503170 - Attachment is obsolete: true
Attachment #8503171 - Attachment is obsolete: true
Attachment #8503172 - Attachment is obsolete: true
Attachment #8503173 - Attachment is obsolete: true
Attachment #8503174 - Attachment is obsolete: true
Attachment #8503175 - Attachment is obsolete: true
Attachment #8503176 - Attachment is obsolete: true
Attachment #8503177 - Attachment is obsolete: true
Attachment #8503178 - Attachment is obsolete: true
Attachment #8506909 - Flags: review?(shuang)
Attachment #8506909 - Flags: feedback?(btian)
Attachment #8506911 - Flags: review?(shuang)
Attachment #8506911 - Flags: feedback?(btian)
Attachment #8506915 - Flags: review?(shuang)
Attachment #8506915 - Flags: feedback?(btian)
Attachment #8506917 - Flags: review?(shuang)
Attachment #8506917 - Flags: feedback?(btian)
This patch will enable the Bluetooth daemon backend. It's not supposed to land now.
Comment on attachment 8506909 [details] [diff] [review]
[01] Bug 1073548: Support Bluetooth daemon in build system

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

I'm not the best person to review configure.in. So redirect to Mike.

::: configure.in
@@ +253,5 @@
> +                MOZ_B2G_BT=1
> +                MOZ_B2G_BT_BLUEDROID=1
> +            fi
> +            if test -d "$gonkdir/system/bluetoothd"; then
> +                MOZ_B2G_BT=1

Maybe we only need to set MOZ_B2G_BT for the case when bluez/bluedroid folders exist?

@@ +266,5 @@
>          AC_SUBST(MOZ_OMX_DECODER)
>          MOZ_OMX_ENCODER=1
>          AC_SUBST(MOZ_OMX_ENCODER)
>          AC_DEFINE(MOZ_OMX_ENCODER)
> +        MOZ_FMP4=1

I suggest let's leave it to another patch to clean up. I already inform the one who breaks this line.

@@ +287,5 @@
>          AC_DEFINE(MOZ_OMX_ENCODER)
>          MOZ_AUDIO_OFFLOAD=1
>          AC_SUBST(MOZ_AUDIO_OFFLOAD)
>          AC_DEFINE(MOZ_AUDIO_OFFLOAD)
> +        MOZ_FMP4=1

Ditto.
Attachment #8506909 - Flags: review?(shuang)
Attachment #8506909 - Flags: review?(mh+mozilla)
Attachment #8506909 - Flags: feedback?(btian)
Attachment #8506909 - Flags: feedback+
Changes since v1:

  - added missing files
Attachment #8506912 - Attachment is obsolete: true
Attachment #8506912 - Flags: review?(shuang)
Attachment #8506912 - Flags: feedback?(btian)
Attachment #8508577 - Flags: review?(shuang)
Attachment #8508577 - Flags: feedback?(btian)
Comment on attachment 8506911 [details] [diff] [review]
[02] Bug 1073548: Add |mozilla::ipc::BluetoothDaemonConnection|

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

I will revisit in detail again. A few questions may your feedback first.

::: ipc/bluetooth/BluetoothDaemonConnection.cpp
@@ +19,5 @@
> +#endif
> +
> +#if defined(MOZ_WIDGET_GONK)
> +#include <android/log.h>
> +#define CHROMIUM_LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "I/O", args);

Change "I/O" to "BluetoothDaemonConnection" will be clear.

@@ +29,5 @@
> +
> +namespace mozilla {
> +namespace ipc {
> +
> +static const char sBluetoothdSocket[] = "bluez_hal_socket";

Please add some comments about bluez_hal_socket or change variable name to sBluetoothdSocketPath.

@@ +43,5 @@
> +{
> +  uint8_t* data = Append(HEADER_SIZE);
> +  MOZ_ASSERT(data);
> +
> +  // Setup PDU header

Can we add PDU format and at the beginning of this file?  It might be better than nothing documented.

@@ +128,5 @@
> +  return mReceivedFd.forget();
> +}
> +
> +nsresult
> +BluetoothDaemonPDU::UpdateHeader()

When do we need to call this function?

@@ +229,5 @@
> +  MOZ_ASSERT(mConnection);
> +  MOZ_ASSERT(mConsumer);
> +
> +  /* There's only one PDU for receiving, which we reuse everytime */
> +  mPDU = new BluetoothDaemonPDU(1<<16);

Is this payload size? Can we have comments for PDU format in general or a clear variable name?

@@ +308,5 @@
> +  }
> +
> +  SetFd(fd);
> +
> +  // Sonnect socket to address

nit: /Sonnect/Connect

@@ +475,5 @@
> +    GetIO()->Connect(sBluetoothdSocket);
> +  }
> +};
> +
> +#if 0

Why do we have "#if 0"?

::: ipc/bluetooth/BluetoothDaemonConnection.h
@@ +80,5 @@
> +  {
> +    return Read(&aValue, sizeof(aValue));
> +  }
> +*/
> +

I'm not quite sure why do we need to keep these comment-out functions?

@@ +118,5 @@
> +  {
> +    return Write(&aValue, sizeof(aValue));
> +  }
> +*/
> +

I'm not quite sure why we need to keep these comment-out functions?

@@ +131,5 @@
> +  size_t mSize;
> +  size_t mOffset;
> +  size_t mAvailableSpace;
> +  nsAutoArrayPtr<uint8_t> mData;
> +  */

I'm not quite sure why we need to keep these comment-out functions?
Hi

(In reply to Shawn Huang [:shawnjohnjr] from comment #23)
> 
> ::: ipc/bluetooth/BluetoothDaemonConnection.cpp
> @@ +19,5 @@
> > +#endif
> > +
> > +#if defined(MOZ_WIDGET_GONK)
> > +#include <android/log.h>
> > +#define CHROMIUM_LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "I/O", args);
> 
> Change "I/O" to "BluetoothDaemonConnection" will be clear.

"I/O" is shorter when reading the logcat. Other users of the socket IPC code either use "I/O" or "Gonk", so I'd prefer to go with "I/O" here.

> @@ +29,5 @@
> > +
> > +namespace mozilla {
> > +namespace ipc {
> > +
> > +static const char sBluetoothdSocket[] = "bluez_hal_socket";
> 
> Please add some comments about bluez_hal_socket or change variable name to
> sBluetoothdSocketPath.

OK
 
> @@ +43,5 @@
> > +{
> > +  uint8_t* data = Append(HEADER_SIZE);
> > +  MOZ_ASSERT(data);
> > +
> > +  // Setup PDU header
> 
> Can we add PDU format and at the beginning of this file?  It might be better
> than nothing documented.

Sure. I'll add a description of the PDU and a link to the BlueZ document. 

> @@ +128,5 @@
> > +  return mReceivedFd.forget();
> > +}
> > +
> > +nsresult
> > +BluetoothDaemonPDU::UpdateHeader()
> 
> When do we need to call this function?

The PDU data structure maintains the length of the internal data buffer which contains the payload. Before we send the PDU, the payload length must be set in the PDU header, which is done by |UpdateHeader|. It's called by the backend code in 'bluetooth/bluedroid' when the size of the payload changes. See BluetoothDaemonInterface.cpp near line 1060.

> @@ +229,5 @@
> > +  MOZ_ASSERT(mConnection);
> > +  MOZ_ASSERT(mConsumer);
> > +
> > +  /* There's only one PDU for receiving, which we reuse everytime */
> > +  mPDU = new BluetoothDaemonPDU(1<<16);
> 
> Is this payload size? Can we have comments for PDU format in general or a
> clear variable name?

Right, this is the (maximum) payload size. I'll add a constant to hold this value.

> @@ +308,5 @@
> > +  }
> > +
> > +  SetFd(fd);
> > +
> > +  // Sonnect socket to address
> 
> nit: /Sonnect/Connect

OK :)

> @@ +475,5 @@
> > +    GetIO()->Connect(sBluetoothdSocket);
> > +  }
> > +};
> > +
> > +#if 0
> 
> Why do we have "#if 0"?

Sorry, about that. I should have cleaned up the code better; here and below. I moved code around several times, and I guess this was left from a former revision.

> 
> ::: ipc/bluetooth/BluetoothDaemonConnection.h
> @@ +80,5 @@
> > +  {
> > +    return Read(&aValue, sizeof(aValue));
> > +  }
> > +*/
> > +
> 
> I'm not quite sure why do we need to keep these comment-out functions?
> 
> @@ +118,5 @@
> > +  {
> > +    return Write(&aValue, sizeof(aValue));
> > +  }
> > +*/
> > +
> 
> I'm not quite sure why we need to keep these comment-out functions?
> 
> @@ +131,5 @@
> > +  size_t mSize;
> > +  size_t mOffset;
> > +  size_t mAvailableSpace;
> > +  nsAutoArrayPtr<uint8_t> mData;
> > +  */
> 
> I'm not quite sure why we need to keep these comment-out functions?
Changes since v1:

  - added documentation for PDU and link to full spec
  - added constant for maximum PDU size
  - cleaned up naming of socket address
  - fixed typos
  - removed out-commented code
Attachment #8506911 - Attachment is obsolete: true
Attachment #8506911 - Flags: review?(shuang)
Attachment #8506911 - Flags: feedback?(btian)
Attachment #8508738 - Flags: review?(shuang)
Attachment #8508738 - Flags: feedback?(btian)
Blocks: 1087195
Blocks: 1087196
Blocks: 1087198
Attachment #8506909 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8508738 [details] [diff] [review]
[02] Bug 1073548: Add |mozilla::ipc::BluetoothDaemonConnection| (v2)

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

Overall, it looks good to me. I only have a few questions.

::: ipc/bluetooth/BluetoothDaemonConnection.cpp
@@ +59,5 @@
> +{ }
> +
> +ssize_t
> +BluetoothDaemonPDU::Send(int aFd)
> +{

MOZ_ASSERT(aFd >= 0);

@@ +60,5 @@
> +
> +ssize_t
> +BluetoothDaemonPDU::Send(int aFd)
> +{
> +  struct iovec iv;

memset(&iv, 0, sizeof(iv));

@@ +85,5 @@
> +      ((_cmsghdr)->cmsg_type == SCM_RIGHTS) )
> +
> +ssize_t
> +BluetoothDaemonPDU::Receive(int aFd)
> +{

MOZ_ASSERT(aFd >= 0);

@@ +86,5 @@
> +
> +ssize_t
> +BluetoothDaemonPDU::Receive(int aFd)
> +{
> +  struct iovec iv;

memset(&iv, 0, sizeof(iv));

@@ +139,5 @@
> +    return NS_ERROR_ILLEGAL_VALUE;
> +  }
> +  uint16_t len16 = static_cast<uint16_t>(len);
> +
> +  memcpy(GetData(2), &len16, sizeof(len16));

Please add comment to explain '2' represents for?

@@ +312,5 @@
> +
> +  SetFd(fd);
> +
> +  // Connect socket to address
> +

nit:Remove the empty line

@@ +374,5 @@
> +BluetoothDaemonConnectionIO::OnSocketCanReceiveWithoutBlocking()
> +{
> +  MOZ_ASSERT(MessageLoopForIO::current() == GetIOLoop());
> +  MOZ_ASSERT(GetConnectionStatus() == SOCKET_IS_CONNECTED);
> +

Add MOZ_ASSERT(!mShuttingDownOnIOThread);

@@ +388,5 @@
> +
> +nsresult
> +BluetoothDaemonConnectionIO::SendPendingData(int aFd)
> +{
> +  while (HasPendingData()) {

Add MOZ_ASSERT(aFd >= 0); before while loop

@@ +400,5 @@
> +      /* I/O is currently blocked; try again later */
> +      return NS_OK;
> +    }
> +
> +    mOutgoingQ.RemoveElementAt(0);

if (!outgoing->GetSize()) { 
  mOutgoingQ.RemoveElementAt(0);
  delete outgoing;
}
Can we add the check?
Attachment #8508738 - Flags: review?(shuang)
> > +ssize_t
> > +BluetoothDaemonPDU::Send(int aFd)
> > +{
> 
> MOZ_ASSERT(aFd >= 0);

Regarding all these fd checks: The test whether file descriptors are valid is ultimately done within the kernel by |sendmsg| and |recvmsg|. I'd like give them a chance to handle file descriptors before failing in Gecko. I'll added an assertion test for (errno != EBADF) if |sendmsg| or |recvmsg| fails. This way, both have a chance to test the validity of the fd, and Gecko still fails on this internal error.
 
> @@ +60,5 @@
> > +
> > +ssize_t
> > +BluetoothDaemonPDU::Send(int aFd)
> > +{
> > +  struct iovec iv;
> 
> memset(&iv, 0, sizeof(iv));

OK, here and below.

> @@ +139,5 @@
> > +    return NS_ERROR_ILLEGAL_VALUE;
> > +  }
> > +  uint16_t len16 = static_cast<uint16_t>(len);
> > +
> > +  memcpy(GetData(2), &len16, sizeof(len16));
> 
> Please add comment to explain '2' represents for?

I added more constants to the PDU class and replaced all these literal values.

> 
> @@ +312,5 @@
> > +
> > +  SetFd(fd);
> > +
> > +  // Connect socket to address
> > +
> 
> nit:Remove the empty line

OK.

> 
> @@ +374,5 @@
> > +BluetoothDaemonConnectionIO::OnSocketCanReceiveWithoutBlocking()
> > +{
> > +  MOZ_ASSERT(MessageLoopForIO::current() == GetIOLoop());
> > +  MOZ_ASSERT(GetConnectionStatus() == SOCKET_IS_CONNECTED);
> > +
> 
> Add MOZ_ASSERT(!mShuttingDownOnIOThread);

OK.

> 
> @@ +388,5 @@
> > +
> > +nsresult
> > +BluetoothDaemonConnectionIO::SendPendingData(int aFd)
> > +{
> > +  while (HasPendingData()) {
> 
> Add MOZ_ASSERT(aFd >= 0); before while loop

See my comment about file-descriptor validity above.
 
> @@ +400,5 @@
> > +      /* I/O is currently blocked; try again later */
> > +      return NS_OK;
> > +    }
> > +
> > +    mOutgoingQ.RemoveElementAt(0);
> 
> if (!outgoing->GetSize()) { 
>   mOutgoingQ.RemoveElementAt(0);
>   delete outgoing;
> }
> Can we add the check?

According to the underlying socket's connection mode, |sendmsg| should _always_ send the complete PDU. So it's an error if |GetSize| returns anything but 0 here. I'll add an assertion test.
Changes since v2:

  - fail when sending/receiving with invalid file descriptors
  - consume PDU's data after success full sending and check if all data has been sent
  - clear iovec structures
  - replace literal values by constants
  - validate I/O thread state
  - comment cleanups
Attachment #8508738 - Attachment is obsolete: true
Attachment #8508738 - Flags: feedback?(btian)
Attachment #8509546 - Flags: review?(shuang)
Attachment #8509546 - Flags: feedback?(btian)
Comment on attachment 8509546 [details] [diff] [review]
[02] Bug 1073548: Add |mozilla::ipc::BluetoothDaemonConnection| (v3)

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

::: ipc/bluetooth/BluetoothDaemonConnection.cpp
@@ +49,5 @@
> +
> +  // Setup PDU header
> +  data[OFF_SERVICE] = aService;
> +  data[OFF_OPCODE] = aOpcode;
> +  memcpy(data+OFF_LENGTH, &aPayloadSize, sizeof(aPayloadSize));

nit: 'data + OFF_LENGTH', empty space
Attachment #8509546 - Flags: review?(shuang) → review+
(In reply to Shawn Huang [:shawnjohnjr] from comment #21)
> Comment on attachment 8506909 [details] [diff] [review]
> [01] Bug 1073548: Support Bluetooth daemon in build system
> 
> Review of attachment 8506909 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not the best person to review configure.in. So redirect to Mike.
> 
> ::: configure.in
> @@ +253,5 @@
> > +                MOZ_B2G_BT=1
> > +                MOZ_B2G_BT_BLUEDROID=1
> > +            fi
> > +            if test -d "$gonkdir/system/bluetoothd"; then
> > +                MOZ_B2G_BT=1
> 
> Maybe we only need to set MOZ_B2G_BT for the case when bluez/bluedroid
> folders exist?
Hi Thomas,
Just want to confirm one thing.
I saw Patch 6, the comments mentioned
B2G Bluetooth daemon:
MOZ_B2G_BT and MOZ_B2G_BT_DAEMON are both defined;
MOZ_B2G_BLUEZ or MOZ_B2G_BLUEDROID are not defined.

But I thought both external/bluetooth/bluedroid and system/bluetoothd can be co-existed at the same time?
If bluedroid/bluetoothd folder co-exists, MOZ_B2G_BLUEDROID shall not be defined.

Following the comment in Patch 6, it's possible in configure.in, MOZ_B2G_BT=1, MOZ_B2G_BLUEDROID=1, MOZ_B2G_BT_DAEMON=1, because bluetoothd still need to call bluedroid library, so we still need to build external/bluetooth/bluedroid.
Comment on attachment 8506917 [details] [diff] [review]
[06] Bug 1073548: Add support for Bluetooth daemon backend

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

LGTM.
Attachment #8506917 - Flags: review?(shuang) → review+
> > Maybe we only need to set MOZ_B2G_BT for the case when bluez/bluedroid
> > folders exist?
> Hi Thomas,
> Just want to confirm one thing.
> I saw Patch 6, the comments mentioned
> B2G Bluetooth daemon:
> MOZ_B2G_BT and MOZ_B2G_BT_DAEMON are both defined;
> MOZ_B2G_BLUEZ or MOZ_B2G_BLUEDROID are not defined.
> 
> But I thought both external/bluetooth/bluedroid and system/bluetoothd can be
> co-existed at the same time?
> If bluedroid/bluetoothd folder co-exists, MOZ_B2G_BLUEDROID shall not be
> defined.
> 
> Following the comment in Patch 6, it's possible in configure.in,
> MOZ_B2G_BT=1, MOZ_B2G_BLUEDROID=1, MOZ_B2G_BT_DAEMON=1, because bluetoothd
> still need to call bluedroid library, so we still need to build
> external/bluetooth/bluedroid.

I don't really understand the comment, so let me explain the idea of the current patch [01].

You can either build support for BlueZ *or* Bluedroid at the same time. If 'external/bluetooth/bluez', BlueZ is choosen, otherwise Bluedroid is tested. So BlueZ currently has preference over Bluedroid. For Bluedroid, there are again 2 cases: using the daemon, or building support in Gecko. If 'external/bluetooth/bluedroid' exists, direct usage is supported; if 'system/bluetoothd' exists, the daemon is build. Both options are not really independent from each other, as the daemon always requires Bluedroid. I guess, the current configure.in doesn't refrect this very well.

Regarding the flags: MOZ_B2G_BT is the general flag for Bluetooth. MOZ_B2G_BLUEZ is defined if BlueZ shall be used. *Only* MOZ_B2G_BLUEDROID is defined if Bluedroid shall be used directly, In addition, MOZ_B2G_DAEMON is defined if the daemon shall be used. BLUEDROID and DAEMON are currently defined at the same time, with DAEMON being an extension.

My original plan was to separate internal Bluedroid and daemon, but that would have added a lot of duplicated code to Gecko's backend. I guess the comments in [06] still refer to this approach. I'll update the comments to match the current code.
Changes since v1:

  - make daemon depend on Bluedroid
  - don't cleanup configure.in
Attachment #8506909 - Attachment is obsolete: true
Attachment #8510922 - Flags: review+
Changes since v3:

  - added whitespaces around operator
Attachment #8509546 - Attachment is obsolete: true
Attachment #8509546 - Flags: feedback?(btian)
Attachment #8510923 - Flags: review+
Changes since v1:

  - fixed comments
Attachment #8506917 - Attachment is obsolete: true
Attachment #8506917 - Flags: feedback?(btian)
Attachment #8510924 - Flags: review+
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #32)
> > > Maybe we only need to set MOZ_B2G_BT for the case when bluez/bluedroid
> > > folders exist?
> > Hi Thomas,
> > Just want to confirm one thing.
> > I saw Patch 6, the comments mentioned
> > B2G Bluetooth daemon:
> > MOZ_B2G_BT and MOZ_B2G_BT_DAEMON are both defined;
> > MOZ_B2G_BLUEZ or MOZ_B2G_BLUEDROID are not defined.
> My original plan was to separate internal Bluedroid and daemon, but that
> would have added a lot of duplicated code to Gecko's backend. I guess the
> comments in [06] still refer to this approach. I'll update the comments to
> match the current code.
Yes, thanks for the clarification. I'm confused at that moment. Because the comment said "MOZ_B2G_BLUEZ or MOZ_B2G_BLUEDROID are not defined".
Comment on attachment 8510924 [details] [diff] [review]
[06] Bug 1073548: Add support for Bluetooth daemon backend (v2)

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

::: dom/bluetooth/BluetoothInterface.cpp
@@ +99,5 @@
>  #ifdef MOZ_B2G_BT_BLUEDROID
>    return BluetoothHALInterface::GetInstance();
>  #else
> +#ifdef MOZ_B2G_BT_DAEMON
> +  return BluetoothDaemonInterface::GetInstance();

We shall move this ifdef up before '#ifdef MOZ_B2G_BT_BLUEDROID', right?
(In reply to Shawn Huang [:shawnjohnjr] from comment #37)
> Comment on attachment 8510924 [details] [diff] [review]
> [06] Bug 1073548: Add support for Bluetooth daemon backend (v2)
> > +#ifdef MOZ_B2G_BT_DAEMON
> > +  return BluetoothDaemonInterface::GetInstance();
> 
> We shall move this ifdef up before '#ifdef MOZ_B2G_BT_BLUEDROID', right?
Opps, I did not see Attachment 8506918 [details] [diff] did this. :)
(In reply to Shawn Huang [:shawnjohnjr] from comment #38)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #37)
> > Comment on attachment 8510924 [details] [diff] [review]
> > [06] Bug 1073548: Add support for Bluetooth daemon backend (v2)
> > > +#ifdef MOZ_B2G_BT_DAEMON
> > > +  return BluetoothDaemonInterface::GetInstance();
> > 
> > We shall move this ifdef up before '#ifdef MOZ_B2G_BT_BLUEDROID', right?
> Opps, I did not see Attachment 8506918 [details] [diff] did this. :)

Right, but that will come a lot later. I first want to land all pieces (daemon, Gecko, etc) for the Flame; then make sure it works on other platforms as well. And once everything's in place and working, enable it for everyone. This will hopefully be the least disruptive way to land these changes.
Comment on attachment 8508577 [details] [diff] [review]
[03] Bug 1073548: Add core interfaces and Setup module for Bluetooth daemon (v2)

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

I have a few questions, i'm happy to review again after patch revision.

::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp
@@ +13,5 @@
> +//
> +
> +nsresult
> +Convert(uint8_t aIn, BluetoothStatus& aOut)
> +{

Please add comments to explain the purpose of this function.

::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.h
@@ +759,5 @@
> +{
> +public:
> +  typedef typename ObjectWrapper::ObjectType  ObjectType;
> +  typedef BluetoothDaemonNotificationRunnable3<ObjectWrapper, Res,
> +                                            Tin1, Tin2, Tin3,

nit: indent

::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
@@ +23,5 @@
> +
> +class BluetoothDaemonSetupModule
> +{
> +public:
> +  virtual nsresult Send(BluetoothDaemonPDU* aPDU, void* aUserData) = 0;

Will it be consistent to use  the name|aPayload| here? As you documented in BluetoothDaemonConnection.h.

@@ +88,5 @@
> +  }
> +
> +protected:
> +
> +  void HandleSvc(const BluetoothDaemonPDUHeader& aHeader,

Please add some comments to explain the purpose of this function. I'm a bit confused about 'Service' here.

@@ +95,5 @@
> +    static void (BluetoothDaemonSetupModule::* const HandleRsp[])(
> +      const BluetoothDaemonPDUHeader&,
> +      BluetoothDaemonPDU&,
> +      BluetoothSetupResultHandler*) = {
> +      [0] = &BluetoothDaemonSetupModule::ErrorRsp,

Can we build this using older GCC version on flatfish?

@@ +209,5 @@
> +private:
> +  void HandleSetupSvc(const BluetoothDaemonPDUHeader& aHeader,
> +                      BluetoothDaemonPDU& aPDU, void* aUserData);
> +
> +  BluetoothDaemonConnection* mConnection;

OK. I think using raw pointer here is safe enough.

@@ +243,5 @@
> +BluetoothDaemonProtocol::Handle(BluetoothDaemonPDU& aPDU)
> +{
> +  static void (BluetoothDaemonProtocol::* const HandleSvc[])(
> +    const BluetoothDaemonPDUHeader&, BluetoothDaemonPDU&, void*) = {
> +    [0] = &BluetoothDaemonProtocol::HandleSetupSvc

Need to confirm this can be supported if we build flatfish.

@@ +271,5 @@
> +{
> +  MOZ_ASSERT(!NS_IsMainThread());
> +
> +  if (aHeader.mOpcode & 0x80) {
> +    return nullptr; // Ignore notifications

I checked hal-ipc-api.txt, it said "Notification opcodes start from 0x80". If I understand correctly,|FetchUserData| only handles incoming PDU data for command response, and ignore notification from Daemon.

@@ +477,5 @@
> +      }
> +      break;
> +
> +    case NTF_CHANNEL:
> +      {

nit: case NTF_CHANNEL: {

@@ +495,5 @@
> +
> +void
> +BluetoothDaemonInterface::OnConnectError(enum Channel aChannel)
> +{
> +  // TODO: handle channels correctly

What's the mechanism to handle NTF/CMD channels in onConnectError?

@@ +499,5 @@
> +  // TODO: handle channels correctly
> +
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(!mResultHandlerQ.IsEmpty());
> +

We should have logs even for production environment for the reason we cannot open CMD/NTF channel. It seems fatal.

@@ +513,5 @@
> +BluetoothDaemonInterface::OnDisconnect(enum Channel aChannel)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(!mResultHandlerQ.IsEmpty());
> +

We should have logs even for production environment for the reason CMD/NTF channels are closed.

@@ +520,5 @@
> +      // Cleanup, step 4: Close command channel
> +      mCmdChannel->CloseSocket();
> +      break;
> +    case CMD_CHANNEL:
> +      {

nit: case CMD_CHANNEL: {, no new line

@@ +592,5 @@
> +    }
> +  }
> +
> +  BluetoothDaemonInterface* mInterface;
> +  nsRefPtr<BluetoothResultHandler> mRes;

I did not see any |mRes| has been used in CleanupResultHandler. Maybe something missed?
Attachment #8508577 - Flags: review?(shuang)
Hi
 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp
> @@ +13,5 @@
> > +//
> > +
> > +nsresult
> > +Convert(uint8_t aIn, BluetoothStatus& aOut)
> > +{
> 
> Please add comments to explain the purpose of this function.

More functions will be added in later patches, so I added a comment to the top of the block.


> ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
> @@ +23,5 @@
> > +
> > +class BluetoothDaemonSetupModule
> > +{
> > +public:
> > +  virtual nsresult Send(BluetoothDaemonPDU* aPDU, void* aUserData) = 0;
> 
> Will it be consistent to use  the name|aPayload| here? As you documented in
> BluetoothDaemonConnection.h.

aPDU is the whole PDU, including the 4-byte header. Payload would only be the rest.


> 
> @@ +88,5 @@
> > +  }
> > +
> > +protected:
> > +
> > +  void HandleSvc(const BluetoothDaemonPDUHeader& aHeader,
> 
> Please add some comments to explain the purpose of this function. I'm a bit
> confused about 'Service' here.

I added a comment. Bluetooth profiles are represented by modules in the HAL IPC protocols. Modules are also called services. So a service is roughly equivalent to a BT profile; except for the service with id 0x00, which handles protocol-internal operations.


> @@ +95,5 @@
> > +    static void (BluetoothDaemonSetupModule::* const HandleRsp[])(
> > +      const BluetoothDaemonPDUHeader&,
> > +      BluetoothDaemonPDU&,
> > +      BluetoothSetupResultHandler*) = {
> > +      [0] = &BluetoothDaemonSetupModule::ErrorRsp,
> 
> Can we build this using older GCC version on flatfish?

I wanted to test this, but there are several other bugs with patches. I thought to wait until these patches have landed, so I don't have to juggle with so many patch files. But I can test this now.


> 
> @@ +271,5 @@
> > +{
> > +  MOZ_ASSERT(!NS_IsMainThread());
> > +
> > +  if (aHeader.mOpcode & 0x80) {
> > +    return nullptr; // Ignore notifications
> 
> I checked hal-ipc-api.txt, it said "Notification opcodes start from 0x80".
> If I understand correctly,|FetchUserData| only handles incoming PDU data for
> command response, and ignore notification from Daemon.

In our case, 'user data' is the result handler from the caller. We need to store it while the command/response is being processed by the daemon. So we put it into |mUserDataQ| before sending and fetch it from the queue when the response arrives. This will work because commands/responses are always processed in pairs.

However, there could also be a notifications being received. There are no result handlers for notifications obviously. So |FetchUserData| return |nullptr| for all notifications.


> @@ +495,5 @@
> > +
> > +void
> > +BluetoothDaemonInterface::OnConnectError(enum Channel aChannel)
> > +{
> > +  // TODO: handle channels correctly
> 
> What's the mechanism to handle NTF/CMD channels in onConnectError?

The HAL IPC protocol requires us to establish two connections to the daemon. If we fail, we should cleanup the other connection (if any). I'll add some code for that.


> @@ +499,5 @@
> > +  // TODO: handle channels correctly
> > +
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  MOZ_ASSERT(!mResultHandlerQ.IsEmpty());
> > +
> 
> We should have logs even for production environment for the reason we cannot
> open CMD/NTF channel. It seems fatal.

The code will output an error message when the respective system calls (connect, accept, etc) fail. Is that sufficient?


> @@ +520,5 @@
> > +      // Cleanup, step 4: Close command channel
> > +      mCmdChannel->CloseSocket();
> > +      break;
> > +    case CMD_CHANNEL:
> > +      {
> 
> nit: case CMD_CHANNEL: {, no new line
> 
> @@ +592,5 @@
> > +    }
> > +  }
> > +
> > +  BluetoothDaemonInterface* mInterface;
> > +  nsRefPtr<BluetoothResultHandler> mRes;
> 
> I did not see any |mRes| has been used in CleanupResultHandler. Maybe
> something missed?

No, it's actually not being used. :D
> 
> @@ +513,5 @@
> > +BluetoothDaemonInterface::OnDisconnect(enum Channel aChannel)
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  MOZ_ASSERT(!mResultHandlerQ.IsEmpty());
> > +
> 
> We should have logs even for production environment for the reason CMD/NTF
> channels are closed.

This is the legal disconnect. I think we should not log anything here, because it's part of the regular operation.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #42)
> > 
> > @@ +513,5 @@
> > > +BluetoothDaemonInterface::OnDisconnect(enum Channel aChannel)
> > > +{
> > > +  MOZ_ASSERT(NS_IsMainThread());
> > > +  MOZ_ASSERT(!mResultHandlerQ.IsEmpty());
> > > +
> > 
> > We should have logs even for production environment for the reason CMD/NTF
> > channels are closed.
> 
> This is the legal disconnect. I think we should not log anything here,
> because it's part of the regular operation.

Ok, no problem.
> > @@ +95,5 @@
> > > +    static void (BluetoothDaemonSetupModule::* const HandleRsp[])(
> > > +      const BluetoothDaemonPDUHeader&,
> > > +      BluetoothDaemonPDU&,
> > > +      BluetoothSetupResultHandler*) = {
> > > +      [0] = &BluetoothDaemonSetupModule::ErrorRsp,
> > 
> > Can we build this using older GCC version on flatfish?
> 
> I wanted to test this, but there are several other bugs with patches. I
> thought to wait until these patches have landed, so I don't have to juggle
> with so many patch files. But I can test this now.
> 

Hmm, :/ Flatfish currently doesn't build because of an error in the media code. I'll try again later this week.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #41)
> Hi
>  
> > ::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp
> 
> > ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
> > @@ +23,5 @@
> > > +
> > > +class BluetoothDaemonSetupModule
> > > +{
> > > +public:
> > > +  virtual nsresult Send(BluetoothDaemonPDU* aPDU, void* aUserData) = 0;
> > 
> > Will it be consistent to use  the name|aPayload| here? As you documented in
> > BluetoothDaemonConnection.h.
> 
> aPDU is the whole PDU, including the 4-byte header. Payload would only be
> the rest.
hmm, I thought I mean replace |aUserData| as |aPayload|.

> > 
> > @@ +88,5 @@
> > > +  }
> > > +
> > > +protected:
> > > +
> > > +  void HandleSvc(const BluetoothDaemonPDUHeader& aHeader,
> > 
> > Please add some comments to explain the purpose of this function. I'm a bit
> > confused about 'Service' here.
> 
> I added a comment. Bluetooth profiles are represented by modules in the HAL
> IPC protocols. Modules are also called services. So a service is roughly
> equivalent to a BT profile; except for the service with id 0x00, which
> handles protocol-internal operations.
Sure.

> 
> > @@ +95,5 @@
> > > +    static void (BluetoothDaemonSetupModule::* const HandleRsp[])(
> > > +      const BluetoothDaemonPDUHeader&,
> > > +      BluetoothDaemonPDU&,
> > > +      BluetoothSetupResultHandler*) = {
> > > +      [0] = &BluetoothDaemonSetupModule::ErrorRsp,
> > 
> > Can we build this using older GCC version on flatfish?
> 
> I wanted to test this, but there are several other bugs with patches. I
> thought to wait until these patches have landed, so I don't have to juggle
> with so many patch files. But I can test this now.


Let's refine later, we can take care in the later patch.
> > 
> > @@ +271,5 @@
> > > +{
> > > +  MOZ_ASSERT(!NS_IsMainThread());
> > > +
> > > +  if (aHeader.mOpcode & 0x80) {
> > > +    return nullptr; // Ignore notifications
> > 
> > I checked hal-ipc-api.txt, it said "Notification opcodes start from 0x80".
> > If I understand correctly,|FetchUserData| only handles incoming PDU data for
> > command response, and ignore notification from Daemon.
> 
> In our case, 'user data' is the result handler from the caller. We need to
> store it while the command/response is being processed by the daemon. So we
> put it into |mUserDataQ| before sending and fetch it from the queue when the
> response arrives. This will work because commands/responses are always
> processed in pairs.
Ha, OK, so my first question asking to replace |aUserData| to |aPayload| seems strange.
> However, there could also be a notifications being received. There are no
> result handlers for notifications obviously. So |FetchUserData| return
> |nullptr| for all notifications.
OK, make sense.
Changes since v2:

  - added comments to explain purpose of |Convert| functions and |HandleSvc|
  - fixed coding style and indention
  - cleanup |BluetoothDaemonInterface::OnConnectError|
  - removed |mRes| from CleanupResultHandler
Attachment #8508577 - Attachment is obsolete: true
Attachment #8508577 - Flags: feedback?(btian)
Attachment #8512007 - Flags: review?(shuang)
Attachment #8512007 - Flags: feedback?(btian)
Changes since v1:

  - rebased onto patch [03](v3)
Attachment #8506913 - Attachment is obsolete: true
Attachment #8506913 - Flags: review?(shuang)
Attachment #8506913 - Flags: feedback?(btian)
Attachment #8512008 - Flags: review?(shuang)
Attachment #8512008 - Flags: feedback?(btian)
Changes since v1:

  - rebased onto patch [03](v3)
Attachment #8506915 - Attachment is obsolete: true
Attachment #8506915 - Flags: review?(shuang)
Attachment #8506915 - Flags: feedback?(btian)
Attachment #8512009 - Flags: review?(shuang)
Attachment #8512009 - Flags: feedback?(btian)
> > > I checked hal-ipc-api.txt, it said "Notification opcodes start from 0x80".
> > > If I understand correctly,|FetchUserData| only handles incoming PDU data for
> > > command response, and ignore notification from Daemon.
> > 
> > In our case, 'user data' is the result handler from the caller. We need to
> > store it while the command/response is being processed by the daemon. So we
> > put it into |mUserDataQ| before sending and fetch it from the queue when the
> > response arrives. This will work because commands/responses are always
> > processed in pairs.
> Ha, OK, so my first question asking to replace |aUserData| to |aPayload|
> seems strange.

Right, the user data is the result handler, not the PDU's payload.

To give some more insights here: Command PDUs are composed on the main thread and given to the connection. Internally, the PDU is transfered to the I/O thread for sending. The data queue in |mUserDataQ| is located on the I/O thread, so that we can fetch the user data on the I/O thread when the response arrives.

This means that we need to get the user data onto the I/O thread before sending the command. It's transfered with the PDU and set by |BluetoothDaemonConnectionIO::Send| before the PDU is enqueued for sending. |BluetoothDaemonProtocol| implements |BluetoothDaemonPDUConsumer::StoreUserData| for that purpose.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #49)
> This means that we need to get the user data onto the I/O thread before
> sending the command. It's transfered with the PDU and set by
> |BluetoothDaemonConnectionIO::Send| before the PDU is enqueued for sending.
> |BluetoothDaemonProtocol| implements
> |BluetoothDaemonPDUConsumer::StoreUserData| for that purpose.
Why not adding comments like this? Comment 49 is really useful for better understanding. ;)
(In reply to Shawn Huang [:shawnjohnjr] from comment #50)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #49)
> > This means that we need to get the user data onto the I/O thread before
> > sending the command. It's transfered with the PDU and set by
> > |BluetoothDaemonConnectionIO::Send| before the PDU is enqueued for sending.
> > |BluetoothDaemonProtocol| implements
> > |BluetoothDaemonPDUConsumer::StoreUserData| for that purpose.
> Why not adding comments like this? Comment 49 is really useful for better
> understanding. ;)

This gave me a good laugh! :D Yes, you're right. I *should* add this kind of comment to the source code instead of the Bugzilla issue. I think I'll supply another patch that documents the overall scheme of how the daemon backend works.
Comment on attachment 8512008 [details] [diff] [review]
[04] Bug 1073548: Implement Bluetooth Core module for Bluetooth daemon (v2)

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

Overall it looks good to me, some convert functions are similar to BluedroidHALHelper.

::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp
@@ +119,5 @@
> +    CONVERT(0x08, PROPERTY_ADAPTER_BONDED_DEVICES),
> +    CONVERT(0x09, PROPERTY_ADAPTER_DISCOVERY_TIMEOUT),
> +    CONVERT(0x0a, PROPERTY_REMOTE_FRIENDLY_NAME),
> +    CONVERT(0x0b, PROPERTY_REMOTE_RSSI),
> +    CONVERT(0x0c, PROPERTY_REMOTE_VERSION_INFO)

How about 'PROPERTY_REMOTE_TRUST_VALUE'?

@@ +580,5 @@
> +  switch (aOut.mType) {
> +    case PROPERTY_BDNAME:
> +      /* fall through */
> +    case PROPERTY_REMOTE_FRIENDLY_NAME:
> +      {

nit:case PROPERTY_REMOTE_FRIENDLY_NAME: {

@@ +596,5 @@
> +      rv = UnpackPDU<BluetoothAddress>(
> +        aPDU, UnpackConversion<BluetoothAddress, nsAString>(aOut.mString));
> +      break;
> +    case PROPERTY_UUIDS:
> +      {

Ditto

@@ +617,5 @@
> +    case PROPERTY_ADAPTER_SCAN_MODE:
> +      rv = UnpackPDU(aPDU, aOut.mScanMode);
> +      break;
> +    case PROPERTY_ADAPTER_BONDED_DEVICES:
> +      {

Ditto

@@ +634,5 @@
> +        rv = Convert(convertArray, aOut.mStringArray);
> +      }
> +      break;
> +    case PROPERTY_REMOTE_RSSI:
> +      {

Ditto

::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
@@ +579,5 @@
> +    };
> +
> +    MOZ_ASSERT(!NS_IsMainThread());
> +
> +    (this->*(HandleOp[!!(aHeader.mOpcode & 0x80)]))(aHeader, aPDU, aUserData);

I'm not quite sure we need '!!' here.

@@ +778,5 @@
> +    static void (BluetoothDaemonCoreModule::* const HandleRsp[])(
> +      const BluetoothDaemonPDUHeader&,
> +      BluetoothDaemonPDU&,
> +      BluetoothResultHandler*) = {
> +      [0x00] = &BluetoothDaemonCoreModule::ErrorRsp,

I'm a bit surprised we can use index like this. If we can build on gcc 4.6/gcc 4.7, i'm ok with it.
Attachment #8512008 - Flags: review?(shuang) → review+
Hi

> Review of attachment 8512008 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall it looks good to me, some convert functions are similar to
> BluedroidHALHelper.

I want to remove the old HAL code as soon as the daemon is ready and stable. So I thought I make a clear cut and separate both from each other.

> ::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp
> @@ +119,5 @@
> > +    CONVERT(0x08, PROPERTY_ADAPTER_BONDED_DEVICES),
> > +    CONVERT(0x09, PROPERTY_ADAPTER_DISCOVERY_TIMEOUT),
> > +    CONVERT(0x0a, PROPERTY_REMOTE_FRIENDLY_NAME),
> > +    CONVERT(0x0b, PROPERTY_REMOTE_RSSI),
> > +    CONVERT(0x0c, PROPERTY_REMOTE_VERSION_INFO)
> 
> How about 'PROPERTY_REMOTE_TRUST_VALUE'?

That's a CAF extension. The daemon will filter out such values for us. Gecko will only see AOSP values.

> 
> Ditto
> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
> @@ +579,5 @@
> > +    };
> > +
> > +    MOZ_ASSERT(!NS_IsMainThread());
> > +
> > +    (this->*(HandleOp[!!(aHeader.mOpcode & 0x80)]))(aHeader, aPDU, aUserData);
> 
> I'm not quite sure we need '!!' here.

That's a quick way to map 0x00 to 0 and 0x80 to 1.

'value & 0x80' will return either 0x00 or 0x80; depending on the MSB of |value|. But as an index into the array, we want either 0 or 1. Negating once will turn 0x00 into 1 and 0x80 into 0. Negating a second time will now turn 1 to 0, and 0 to 1.

> @@ +778,5 @@
> > +    static void (BluetoothDaemonCoreModule::* const HandleRsp[])(
> > +      const BluetoothDaemonPDUHeader&,
> > +      BluetoothDaemonPDU&,
> > +      BluetoothResultHandler*) = {
> > +      [0x00] = &BluetoothDaemonCoreModule::ErrorRsp,
> 
> I'm a bit surprised we can use index like this. If we can build on gcc
> 4.6/gcc 4.7, i'm ok with it.

It's just hexadecimal notation. :) I though it writing down the IPC protocol values into the array is easier to understand than the decimal notation.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #53)
> Hi
> 
> > Review of attachment 8512008 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Overall it looks good to me, some convert functions are similar to
> > BluedroidHALHelper.
> 
> I want to remove the old HAL code as soon as the daemon is ready and stable.
> So I thought I make a clear cut and separate both from each other.
Sure.
> 
> > ::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp
> > @@ +119,5 @@
> > > +    CONVERT(0x08, PROPERTY_ADAPTER_BONDED_DEVICES),
> > > +    CONVERT(0x09, PROPERTY_ADAPTER_DISCOVERY_TIMEOUT),
> > > +    CONVERT(0x0a, PROPERTY_REMOTE_FRIENDLY_NAME),
> > > +    CONVERT(0x0b, PROPERTY_REMOTE_RSSI),
> > > +    CONVERT(0x0c, PROPERTY_REMOTE_VERSION_INFO)
> > 
> > How about 'PROPERTY_REMOTE_TRUST_VALUE'?
> 
> That's a CAF extension. The daemon will filter out such values for us. Gecko
> will only see AOSP values.
That will be great! We finally can isolate their extension. :)
Comment on attachment 8512009 [details] [diff] [review]
[05] Bug 1073548: Implement Socket module for Bluetooth daemon (v2)

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

::: dom/bluetooth/bluedroid/BluetoothDaemonSocketInterface.cpp
@@ +11,5 @@
> +#include "mozilla/unused.h"
> +
> +BEGIN_BLUETOOTH_NAMESPACE
> +
> +#if 0

Oh! Please remove '#if 0 block'.

@@ +304,5 @@
> +
> +private:
> +  SocketMessageWatcher* mWatcher;
> +};
> +#endif

Please remove '#if 0'.
Attachment #8512009 - Flags: review?(shuang)
Changes since v2:

  - fixed coding style for braces in switch statements
  - fixed designated array initializers on Flatfish
Attachment #8512008 - Attachment is obsolete: true
Attachment #8512008 - Flags: feedback?(btian)
Attachment #8513411 - Flags: review?(shuang)
Changes since v2:

  - removed dead code within '#if 0' blocks
  - fixed designated array initializers on Flatfish
Attachment #8512009 - Attachment is obsolete: true
Attachment #8512009 - Flags: feedback?(btian)
Attachment #8513413 - Flags: review?(shuang)
Comment on attachment 8513411 [details] [diff] [review]
[04] Bug 1073548: Implement Bluetooth Core module for Bluetooth daemon (v3)

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

::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.h
@@ +536,5 @@
>      return rv;
>    }
>    return UnpackPDU(aPDU, aArg2);
>  }
>  

It will be good if you can add one line comment to describe these notification functions PDU. There are so many functions with opcode, and hard to tell the  purpose from parameters. I don't want to enforce to do that now, because it creates re-base efforts, probably add some comments when you have time.

@@ +682,5 @@
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +
> +  /* Read ACL state */

Opcode 0x8a - DUT Mode Receive notification
            Notification parameters: Opcode (2 octets)
                                         Length  (1 octet)
                                         Data (variable)
nit: s/ACL/Data/
Attachment #8513411 - Flags: review?(shuang) → review+
Hi

(In reply to Shawn Huang [:shawnjohnjr] from comment #58)
> Comment on attachment 8513411 [details] [diff] [review]
> [04] Bug 1073548: Implement Bluetooth Core module for Bluetooth daemon (v3)
> 
> Review of attachment 8513411 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.h
> @@ +536,5 @@
> >      return rv;
> >    }
> >    return UnpackPDU(aPDU, aArg2);
> >  }
> >  
> 
> It will be good if you can add one line comment to describe these
> notification functions PDU. There are so many functions with opcode, and
> hard to tell the  purpose from parameters. I don't want to enforce to do
> that now, because it creates re-base efforts, probably add some comments
> when you have time.

You're right about the current unpack code. I'm already working an a cleanup patch that will make the notification runnables backend-agnostic, remove the service and opcode constants, and move unpack functions for special cases close to the code that generates the notification.
Comment on attachment 8513413 [details] [diff] [review]
[05] Bug 1073548: Implement Socket module for Bluetooth daemon (v3)

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

::: dom/bluetooth/bluedroid/BluetoothDaemonSocketInterface.cpp
@@ +5,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "BluetoothDaemonSocketInterface.h"
> +#include "BluetoothSocketMessageWatcher.h"
> +#include "nsClassHashtable.h"

Do we actually use hash table in this file? Or maybe I missed something?

@@ +130,5 @@
> +
> +nsresult
> +BluetoothDaemonSocketModule::AcceptCmd(int aFd,
> +                                       BluetoothSocketResultHandler* aRes)
> +{

MOZ_ASSERT(NS_IsMainThread());

@@ +140,5 @@
> +}
> +
> +nsresult
> +BluetoothDaemonSocketModule::CloseCmd(BluetoothSocketResultHandler* aRes)
> +{

MOZ_ASSERT(NS_IsMainThread());
Attachment #8513413 - Flags: review?(shuang)
Changes since v3:

  - fixed comment
Attachment #8513411 - Attachment is obsolete: true
Attachment #8513520 - Flags: review+
Changes since v3:

  - removed unnecessary include statement
  - added assertions for main thread to cmd methods
Attachment #8513413 - Attachment is obsolete: true
Attachment #8513522 - Flags: review?(shuang)
> @@ +95,5 @@
> > +    static void (BluetoothDaemonSetupModule::* const HandleRsp[])(
> > +      const BluetoothDaemonPDUHeader&,
> > +      BluetoothDaemonPDU&,
> > +      BluetoothSetupResultHandler*) = {
> > +      [0] = &BluetoothDaemonSetupModule::ErrorRsp,
> 
> Can we build this using older GCC version on flatfish?
Don't forget to fix it. :)
(In reply to Shawn Huang [:shawnjohnjr] from comment #63)
> > @@ +95,5 @@
> > > +    static void (BluetoothDaemonSetupModule::* const HandleRsp[])(
> > > +      const BluetoothDaemonPDUHeader&,
> > > +      BluetoothDaemonPDU&,
> > > +      BluetoothSetupResultHandler*) = {
> > > +      [0] = &BluetoothDaemonSetupModule::ErrorRsp,
> > 
> > Can we build this using older GCC version on flatfish?
> Don't forget to fix it. :)

I build on Flatfish and integrated the required changes back into the patch set. Should be fixed in recent patches [04] and [05].
Comment on attachment 8513522 [details] [diff] [review]
[05] Bug 1073548: Implement Socket module for Bluetooth daemon (v4)

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

::: dom/bluetooth/moz.build
@@ +50,5 @@
>                  'bluedroid/BluetoothAvrcpHALInterface.cpp',
>                  'bluedroid/BluetoothDaemonHelpers.cpp',
>                  'bluedroid/BluetoothDaemonInterface.cpp',
>                  'bluedroid/BluetoothDaemonSetupInterface.cpp',
> +                'bluedroid/BluetoothDaemonSocketInterface.cpp',

I'm sorry, I should raise this question while reviewing patch [3]. But does it make sense to add BluetoothDaemon*.cpp files only if we have defined MOZ_B2G_BT_DAEMON?
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #64)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #63)
> > > @@ +95,5 @@
> > > > +    static void (BluetoothDaemonSetupModule::* const HandleRsp[])(
> > > > +      const BluetoothDaemonPDUHeader&,
> > > > +      BluetoothDaemonPDU&,
> > > > +      BluetoothSetupResultHandler*) = {
> > > > +      [0] = &BluetoothDaemonSetupModule::ErrorRsp,
> > > 
> > > Can we build this using older GCC version on flatfish?
> > Don't forget to fix it. :)
> 
> I build on Flatfish and integrated the required changes back into the patch
> set. Should be fixed in recent patches [04] and [05].

Yes, you're right.
(In reply to Shawn Huang [:shawnjohnjr] from comment #65)
> Comment on attachment 8513522 [details] [diff] [review]
> [05] Bug 1073548: Implement Socket module for Bluetooth daemon (v4)
> 
> Review of attachment 8513522 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/moz.build
> @@ +50,5 @@
> >                  'bluedroid/BluetoothAvrcpHALInterface.cpp',
> >                  'bluedroid/BluetoothDaemonHelpers.cpp',
> >                  'bluedroid/BluetoothDaemonInterface.cpp',
> >                  'bluedroid/BluetoothDaemonSetupInterface.cpp',
> > +                'bluedroid/BluetoothDaemonSocketInterface.cpp',
> 
> I'm sorry, I should raise this question while reviewing patch [3]. But does
> it make sense to add BluetoothDaemon*.cpp files only if we have defined
> MOZ_B2G_BT_DAEMON?

These files have no extra dependencies. I thought it makes sense to build them unconditionally, so that build errors will be detected early. We can change it though.
> > I'm sorry, I should raise this question while reviewing patch [3]. But does
> > it make sense to add BluetoothDaemon*.cpp files only if we have defined
> > MOZ_B2G_BT_DAEMON?
> 
> These files have no extra dependencies. I thought it makes sense to build
> them unconditionally, so that build errors will be detected early. We can
> change it though.
Ok, I agree with you to capture errors early.
Blocks: 1091575
Blocks: 1091577
Ryan,

Was this bug missed? The patches have landed in m-c a while ago, but it's still open.

  https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=f72e09357945
Flags: needinfo?(ryanvm)
Apparently I forgot to run mcMerge on that push. Sorry!
Flags: needinfo?(ryanvm)
Blocks: 1095436
Blocks: 1065336
Attachment #8512007 - Flags: feedback?(btian)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: