Closed Bug 1054830 Opened 5 years ago Closed 5 years ago

Gatt client neutral-backend interface

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: shawnjohnjr, Assigned: yrliou)

References

Details

(Whiteboard: [webbt-api])

Attachments

(3 files, 6 obsolete files)

2.08 KB, patch
Details | Diff | Splinter Review
64.73 KB, patch
Details | Diff | Splinter Review
16.25 KB, patch
Details | Diff | Splinter Review
Make Bluetooth Gatt client interface backend neutral
Assignee: nobody → joliu
Blocks: 933357
Whiteboard: [webbt-api]
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Blocks: 1063444
Blocks: 1063449
This patch defines several structures in BluetoothCommon.h which will be used to substitute bluedroid gatt data structures in later patch set.
This patch add
  1. gatt main interface(for init and cleanup)
  2. client interface
  3. result handlers for 1 and 2.
  4. notifications (callback) for 1 and 2.
  5. convert functions needed in previous items
into BluetoothInterface.
This patch implements BluetoothGattManager which involves
  - Init/Cleanup GattInterface
  - notification handling functions for Gatt callbacks
Comment on attachment 8487792 [details] [diff] [review]
Bug 1054830 - Patch1: Define structures to represent bluedroid's gatt data structures.

Hi Thomas,

This bug is for adding Gatt interfaces into BluetoothInterface.
We will start to implement Gatt client APIs based on this interface after this bug.
Could you help to review my patches since you're familiar with BluetoothInterface? ;)

BTW, I know there's plan to move codes out from BluetoothInterface, I haven't done that yet since it's only in bluetooth1 right now.

Thanks,
Jocelyn

Patch summary:
Patch1:
  Defines several structures in BluetoothCommon.h which will be used to substitute bluedroid gatt data structures in later patch set.

Patch2:
  Add
  1. gatt main interface(for init and cleanup)
  2. client interface
  3. result handlers for 1 and 2.
  4. notifications (callback) for 1 and 2.
  5. convert functions needed in previous items
into BluetoothInterface.

Patch3:
  Implements BluetoothGattManager which involves
  - Init/Cleanup GattInterface
  - notification handling functions for Gatt callbacks
Attachment #8487792 - Flags: review?(tzimmermann)
Attachment #8487796 - Flags: review?(tzimmermann)
Attachment #8487797 - Flags: review?(tzimmermann)
Adding a reference to the bug for splitting up |BluetoothInterface|
Depends on: 1061489
Comment on attachment 8487792 [details] [diff] [review]
Bug 1054830 - Patch1: Define structures to represent bluedroid's gatt data structures.

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

r+ with nits.

::: dom/bluetooth2/BluetoothCommon.h
@@ +491,5 @@
> +  uint8_t mAdvData[62];
> +};
> +
> +struct BluetoothGattServiceId {
> +  uint8_t mUuid[16];

Please use BluetoothUuid for all the UUIDs. There is also already a conversion function for this type.

@@ +504,5 @@
> +
> +struct BluetoothGattReadParam {
> +  BluetoothGattServiceId mServiceId;
> +  BluetoothGattCharacteristicId mCharId;
> +  uint8_t mDescriptorId[16];

That's also a UUID, I think.

@@ +505,5 @@
> +struct BluetoothGattReadParam {
> +  BluetoothGattServiceId mServiceId;
> +  BluetoothGattCharacteristicId mCharId;
> +  uint8_t mDescriptorId[16];
> +  uint8_t mValue[400];

If it's not obvious from the GATT spec where the array length comes from, you might want to add a comment.
Attachment #8487792 - Flags: review?(tzimmermann) → review+
Comment on attachment 8487796 [details] [diff] [review]
Bug 1054830 - Patch2: Add gatt main/client interface and notifications into BluetoothInterface.

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

Again, r+ with nits. The major issues are the lack of protection against ANDROID_VERSION, and not using |BluetoothUuid|.

::: dom/bluetooth2/bluedroid/BluetoothInterface.cpp
@@ +235,5 @@
> +{
> +  memcpy(aOut, aIn.uu, sizeof(aIn.uu));
> +
> +  return NS_OK;
> +}

Remove in favor of BluetoothUuid.

@@ +952,5 @@
> +static nsresult
> +Convert(const btgatt_char_id_t& aIn, BluetoothGattCharacteristicId& aOut)
> +{
> +  if (NS_FAILED(Convert(aIn.uuid, aOut.mUuid))) {
> +    return NS_ERROR_ILLEGAL_VALUE;

Please return the result from the failed conversion function. Here and below.

@@ +971,5 @@
> +    return NS_ERROR_ILLEGAL_VALUE;
> +  }
> +
> +  memcpy(&aOut.id, &id, sizeof(aOut.id));
> +  aOut.is_primary = aIn.mIsPrimary;

You should remove these copy operations and convert directly into the fields of aOut.

@@ +992,5 @@
> +
> +static nsresult
> +Convert(const btgatt_read_params_t& aIn, BluetoothGattReadParam& aOut)
> +{
> +

Empty line.

@@ +3805,5 @@
> +  typedef BluetoothNotificationRunnable3<GattNotificationHandlerWrapper,
> +                                         void,
> +                                         int, int, uint8_t[16],
> +                                         int, int, const uint8_t*>
> +    RegisterClientNotification;

With |BluetoothUuid|, you should store BluetoothUuid in the runnable and and pass 'const BluetoothUuid&' to the notification method. Here and below.

@@ +4160,5 @@
> +BluetoothGattClientInterface::~BluetoothGattClientInterface()
> +{ }
> +
> +void
> +BluetoothGattClientInterface::RegisterClient(const uint8_t aUuid[16],

Similar to data structures and notifications, these UUIDs should be of type |BluetoothUuid|.

@@ +4608,5 @@
> +BluetoothGattInterface::Init(
> +  BluetoothGattNotificationHandler* aNotificationHandler,
> +  BluetoothGattResultHandler* aRes)
> +{
> +  static btgatt_client_callbacks_t sGattClientCallbacks = {

'static const'

@@ +4626,5 @@
> +    BluetoothGattCallback::ReadDescriptor,
> +    BluetoothGattCallback::WriteDescriptor
> +  };
> +
> +  static btgatt_server_callbacks_t sGattServerCallbacks = {

'static const'

@@ +4642,5 @@
> +    BluetoothGattCallback::RequestExecWrite,
> +    BluetoothGattCallback::ResponseConfirmation
> +  };
> +
> +  static btgatt_callbacks_t sCallbacks = {

'static const'

::: dom/bluetooth2/bluedroid/BluetoothInterface.h
@@ +13,5 @@
>  #include <hardware/bt_av.h>
>  #if ANDROID_VERSION >= 18
>  #include <hardware/bt_rc.h>
>  #endif
> +#include <hardware/bt_gatt.h>

This won't be available before ANDROID_VERSION >= 18, right? So it need protection by the #if above.

The same is true for most of the code in BluetoothInterface.cpp. Interface methods that are not implemented by the underlying Android HAL, should call the result handler's OnError method with STATUS_UNSUPPORTED.

We had lot's of trouble with incompatible versions for Bluedroid headers. I suggest you build these patches on Flame-kk, Nexus 4, and Flatfish before landing them.

@@ +490,5 @@
>  //
> +// GATT Interface
> +//
> +
> +class BluetoothGattNotificationHandler

Since you're using separate interface classes for GATT client and server, I suggest to create separate notification handlers as well.

@@ +787,5 @@
> +  BluetoothGattInterface(const btgatt_interface_t* aInterface);
> +  ~BluetoothGattInterface();
> +
> +private:
> +  const btgatt_interface_t* mInterface;

ANDROID_VERSION
Attachment #8487796 - Flags: review?(tzimmermann) → review+
Comment on attachment 8487797 [details] [diff] [review]
Bug 1054830 - Patch3: Add init/cleanup and notification handling functions into GattManager.

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

Looks good.

::: dom/bluetooth2/bluedroid/BluetoothGattManager.cpp
@@ +21,5 @@
> +namespace {
> +  StaticRefPtr<BluetoothGattManager> sBluetoothGattManager;
> +  bool sInShutdown = false;
> +  static BluetoothGattInterface* sBluetoothGattInterface;
> +  static BluetoothGattClientInterface* sBluetoothGattClientInterface;

Everything here should use |BluetoothGattManager|, so I suggest to remove these three static variables, and put them into |BluetoothGattManager| instead.

::: dom/bluetooth2/bluedroid/BluetoothGattManager.h
@@ +11,5 @@
> +#include "BluetoothInterface.h"
> +#include "BluetoothProfileManagerBase.h"
> +
> +BEGIN_BLUETOOTH_NAMESPACE
> +class BluetoothGattManager : public nsIObserver

No class inherits from |BluetoothGattManager|, right? Please add MOZ_FINAL before the colon.
Attachment #8487797 - Flags: review?(tzimmermann) → review+
Maybe a word about splitting the files: All interfaces go to BluetoothInterface.{cpp,h}. The GATT code should go to bluedroid/BluetoothGattHALInterface.{cpp,h}; expect for the conversion functions, which go to bluedroid/BluetoothHALHelpers.{cpp,h}. My rule-of-thumb has been to put all trivial conversion functions into header, and all others (like those containing function calls) into the source file.
(In reply to Thomas Zimmermann (on PTO) [:tzimmermann] [:tdz] from comment #8)
> Comment on attachment 8487797 [details] [diff] [review]
> Bug 1054830 - Patch3: Add init/cleanup and notification handling functions
> into GattManager.
> 
> Review of attachment 8487797 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.
> 
> ::: dom/bluetooth2/bluedroid/BluetoothGattManager.cpp
> @@ +21,5 @@
> > +namespace {
> > +  StaticRefPtr<BluetoothGattManager> sBluetoothGattManager;
> > +  bool sInShutdown = false;
> > +  static BluetoothGattInterface* sBluetoothGattInterface;
> > +  static BluetoothGattClientInterface* sBluetoothGattClientInterface;
> 
> Everything here should use |BluetoothGattManager|, so I suggest to remove
> these three static variables, and put them into |BluetoothGattManager|
> instead.
> 
We will use sBluetoothGattInterface and sBluetoothGattClientInterface in CleanupResultHandler.
Assign them to nullptr in Cleanup(), which I missed client one in previous version, sorry about that.

For sInShutdown, I put it here just to conform with other profile managers.
Actually, for those interfaces, I put them here for this reason, too.

According above reasons, I prefer leave them as the same, what's your opinion on this?
Thanks.
Flags: needinfo?(tzimmermann)
Hi

> We will use sBluetoothGattInterface and sBluetoothGattClientInterface in
> CleanupResultHandler.
> Assign them to nullptr in Cleanup(), which I missed client one in previous
> version, sorry about that.
> 
> For sInShutdown, I put it here just to conform with other profile managers.
> Actually, for those interfaces, I put them here for this reason, too.
> 
> According above reasons, I prefer leave them as the same, what's your
> opinion on this?
> Thanks.

I don't have a strong opinion here, but for general design reasons, I think at least |sInShutdown| should be made a member of |BluetoothGattManager|. I guess that in the old manager's code, a lot of different classes shared such variables, so putting them in to global locations made sense. With the new design, this isn't the case any longer and the other manager classes should probably be cleaned up a bit at some point.

For those interface classes, I always thought that it might make sense to not look them up here, but in the |BluedroidService| and pass them as parameters to the manager. Again that's something for a later patch.
Flags: needinfo?(tzimmermann)
Attachment #8495089 - Flags: review?(tzimmermann)
Comment on attachment 8495091 [details] [diff] [review]
Bug 1054830 - Patch2(v2): Implement gatt main/client interface and notifications.

* move gatt code out side of BluetoothInterface
* address review comments
Attachment #8495091 - Flags: review?(tzimmermann)
Comment on attachment 8495094 [details] [diff] [review]
Bug 1054830 - Patch3(v2): Add init/cleanup and notification handling functions into GattManager.

Hi Thomas,

I would like to request review again for these patches after revise it based on current code base and review comments since it's not a small change.
Please kindly review them again.

Thanks,
Jocelyn
Attachment #8495094 - Flags: review?(tzimmermann)
Comment on attachment 8495089 [details] [diff] [review]
Bug 1054830 - Patch1(v2): Define structures to represent bluedroid's gatt data structures.

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

Looks good.

::: dom/bluetooth2/BluetoothCommon.h
@@ +499,5 @@
> +
> +struct BluetoothGattId {
> +  BluetoothUuid mUuid;
> +  uint8_t mInstanceId;
> +};

This type was introduced for better compatibility with Kitkat, right? So it won't support JB.
Attachment #8495089 - Flags: review?(tzimmermann) → review+
Comment on attachment 8495091 [details] [diff] [review]
Bug 1054830 - Patch2(v2): Implement gatt main/client interface and notifications.

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

Just nits.

::: dom/bluetooth2/bluedroid/BluetoothGattHALInterface.cpp
@@ +363,5 @@
> +#endif // ANDROID_VERSION >= 19
> +};
> +
> +//static
> +//BluetoothGattServerNotificationHandler* sGattServerNotificationHandler;

Cleanup needed.

@@ +532,5 @@
> +  int aClientIf, const nsAString& aBdAddr,
> +  bool aIsDirect, BluetoothGattClientResultHandler* aRes)
> +{
> +  bt_status_t status;
> +  bt_bdaddr_t bdAddr;

The compiler will emit a warning about this variable being unused for ANDROID_VERSION < 19. I suggest to declare it within the #if branch. Here and all for similar cases below.

::: dom/bluetooth2/bluedroid/BluetoothGattHALInterface.h
@@ +154,5 @@
> +};
> +
> +// TODO: Add server interface
> +
> +class BluetoothGattHALInterface

MOZ_FINAL

::: dom/bluetooth2/bluedroid/BluetoothHALHelpers.cpp
@@ +264,5 @@
> +{
> +  if (NS_FAILED(Convert(aIn.srvc_id, aOut.mServiceId)) ||
> +      NS_FAILED(Convert(aIn.char_id, aOut.mCharId)) ||
> +      NS_FAILED(Convert(aIn.descr_id, aOut.mDescriptorId))) {
> +    return NS_ERROR_ILLEGAL_VALUE;

Please return the result code of the failed |Convert| operation. Here and below.
Attachment #8495091 - Flags: review?(tzimmermann) → review+
Comment on attachment 8495094 [details] [diff] [review]
Bug 1054830 - Patch3(v2): Add init/cleanup and notification handling functions into GattManager.

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

Looks good.
Attachment #8495094 - Flags: review?(tzimmermann) → review+
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #17)
> Comment on attachment 8495089 [details] [diff] [review]
> Bug 1054830 - Patch1(v2): Define structures to represent bluedroid's gatt
> data structures.
> 
> Review of attachment 8495089 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.
> 
> ::: dom/bluetooth2/BluetoothCommon.h
> @@ +499,5 @@
> > +
> > +struct BluetoothGattId {
> > +  BluetoothUuid mUuid;
> > +  uint8_t mInstanceId;
> > +};
> 
> This type was introduced for better compatibility with Kitkat, right? So it
> won't support JB.

Yes, and we plan to have GATT support start from KK instead of JB.
Thus, we use android version >= 19 for gatt interfaces.
No try server result since try server only builds bluetooth folder.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.