Closed
Bug 1054830
Opened 10 years ago
Closed 10 years ago
Gatt client neutral-backend interface
Categories
(Firefox OS Graveyard :: Bluetooth, defect)
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 | ||
Updated•10 years ago
|
Assignee: nobody → joliu
Updated•10 years ago
|
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Assignee | ||
Comment 1•10 years ago
|
||
This patch defines several structures in BluetoothCommon.h which will be used to substitute bluedroid gatt data structures in later patch set.
Assignee | ||
Comment 2•10 years ago
|
||
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.
Assignee | ||
Comment 3•10 years ago
|
||
This patch implements BluetoothGattManager which involves - Init/Cleanup GattInterface - notification handling functions for Gatt callbacks
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8487796 -
Flags: review?(tzimmermann)
Assignee | ||
Updated•10 years ago
|
Attachment #8487797 -
Flags: review?(tzimmermann)
Comment 5•10 years ago
|
||
Adding a reference to the bug for splitting up |BluetoothInterface|
Depends on: 1061489
Comment 6•10 years ago
|
||
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 7•10 years ago
|
||
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 8•10 years ago
|
||
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+
Comment 9•10 years ago
|
||
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.
Assignee | ||
Comment 10•10 years ago
|
||
(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)
Comment 11•10 years ago
|
||
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)
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8487792 -
Attachment is obsolete: true
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8487796 -
Attachment is obsolete: true
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8487797 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8495089 -
Flags: review?(tzimmermann)
Assignee | ||
Comment 15•10 years ago
|
||
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)
Assignee | ||
Comment 16•10 years ago
|
||
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 17•10 years ago
|
||
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 18•10 years ago
|
||
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 19•10 years ago
|
||
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+
Assignee | ||
Comment 20•10 years ago
|
||
(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.
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8495089 -
Attachment is obsolete: true
Assignee | ||
Comment 22•10 years ago
|
||
* Address review comments
Attachment #8495091 -
Attachment is obsolete: true
Assignee | ||
Comment 23•10 years ago
|
||
Thanks for your time, Thomas!
Attachment #8495094 -
Attachment is obsolete: true
Assignee | ||
Comment 24•10 years ago
|
||
No try server result since try server only builds bluetooth folder.
Keywords: checkin-needed
Comment 25•10 years ago
|
||
b2g-inbound: remote: https://hg.mozilla.org/integration/b2g-inbound/rev/477d1f06e4dc remote: https://hg.mozilla.org/integration/b2g-inbound/rev/ca58327b61f0 remote: https://hg.mozilla.org/integration/b2g-inbound/rev/c631028617ab
Keywords: checkin-needed
Comment 26•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/477d1f06e4dc https://hg.mozilla.org/mozilla-central/rev/ca58327b61f0 https://hg.mozilla.org/mozilla-central/rev/c631028617ab
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•