Closed Bug 1228546 Opened 9 years ago Closed 8 years ago

Implement peripheral mode support for GATT Server API

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

(firefox46 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox46 --- fixed

People

(Reporter: yrliou, Assigned: yrliou)

References

Details

Attachments

(1 file, 6 obsolete files)

[Final] Bug 1228546 - Implement peripheral mode support for GATT API. r=brsun, r=mrbkap
56.84 KB, patch
Details | Diff | Splinter Review 
Currently, our GATT Server API doesn't support peripheral mode, open this bug for tracking this.
* Expose adapter.start/stopAdvertising API to enable/disable advertisement

In this WIP patch, I didn't design and expose API for applications to configure their advertisement data, but only expose a switch for advertising to test our GATT server API.
With this WIP patch, application can enable advertisement and listen to inbound connection requests.
By using this WIP patch, I could use a central GATT client to connect to my peripheral GATT server and read/write values to the server.

TODO: test if we could receive notifications/indications from the server
Depends on: 1228909
(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #1)
> Created attachment 8693418 [details] [diff] [review]
> [WIP] Implement adapter.start/stopAdvertising
> 
> * Expose adapter.start/stopAdvertising API to enable/disable advertisement
> 
> In this WIP patch, I didn't design and expose API for applications to
> configure their advertisement data, but only expose a switch for advertising
> to test our GATT server API.
> With this WIP patch, application can enable advertisement and listen to
> inbound connection requests.
> By using this WIP patch, I could use a central GATT client to connect to my
> peripheral GATT server and read/write values to the server.
> 
> TODO: test if we could receive notifications/indications from the server

Tested and we could subscribe notifications and receive notifications from the server.
- Implement start/stopAdvertising API with custom advData parameter.
Attachment #8693418 - Attachment is obsolete: true
Comment on attachment 8700537 [details] [diff] [review]
Bug 1228546 - Implement peripheral mode support for GATT API.

Hi Bruce,

Please help to review my patch, thanks!
Attachment #8700537 - Flags: review?(brsun)
Comment on attachment 8700537 [details] [diff] [review]
Bug 1228546 - Implement peripheral mode support for GATT API.

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

Some suggestions about the interface and the behavior:
 - It might not be so intuitive to imply listening for incoming connection in |startAdvertising|. Moreover, |startAdvertising| always allows the device to be connectible, which might surprise users if they don't want to make their device connectible. Suggest to use a separated API for listening, and don't automatically make the device connectible by simply calling |startAdvertising|.
 - Although connection management of LE devices are part of GAP, we gather them in |BluetoothGatt| or |BluetoothGattServer| related objects. Suggest move these newly added APIs (i.e. advertising and listening) into |BluetoothGattServer| object. The logic of our API design would be more understandable if we sync our original design.

Please also refer to my comments inline below. Feel free to share your comments as well.

::: dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.cpp
@@ +605,5 @@
>      PackConversion<bool, uint8_t>(aIsTxPowerIncluded),
>      PackConversion<int, int32_t>(aMinInterval),
>      PackConversion<int, int32_t>(aMaxInterval),
>      PackConversion<int, int32_t>(aApperance),
> +    static_cast<uint16_t>(aManufacturerLen * sizeof(uint8_t)),

Not sure about the intention of |sizeof(uint8_t)|. Maybe using |elem_type| of the instance of |nsTArray<T>|?

@@ +606,5 @@
>      PackConversion<int, int32_t>(aMinInterval),
>      PackConversion<int, int32_t>(aMaxInterval),
>      PackConversion<int, int32_t>(aApperance),
> +    static_cast<uint16_t>(aManufacturerLen * sizeof(uint8_t)),
> +    static_cast<uint16_t>(aServiceDataLen * sizeof(uint8_t)), /* Byte Length */

ditto

@@ +607,5 @@
>      PackConversion<int, int32_t>(aMaxInterval),
>      PackConversion<int, int32_t>(aApperance),
> +    static_cast<uint16_t>(aManufacturerLen * sizeof(uint8_t)),
> +    static_cast<uint16_t>(aServiceDataLen * sizeof(uint8_t)), /* Byte Length */
> +    static_cast<uint16_t>(aServiceUuidsLen * sizeof(BluetoothUuid)),

|sizeof(BluetoothUuid)| is not trustworthy. If some extra member fields are added in |BluetoothUuid| someday later, this command would be broken silently. If our IPC protocol is based on HAL, |sizeof(bt_uuid_t.uu)| would be a better choice comparing to |sizeof(BluetoothUuid)|.

@@ +608,5 @@
>      PackConversion<int, int32_t>(aApperance),
> +    static_cast<uint16_t>(aManufacturerLen * sizeof(uint8_t)),
> +    static_cast<uint16_t>(aServiceDataLen * sizeof(uint8_t)), /* Byte Length */
> +    static_cast<uint16_t>(aServiceUuidsLen * sizeof(BluetoothUuid)),
> +    PackArray<uint8_t>(aManufacturerData, aManufacturerLen),

The length calculation is not sync with other parameter of the function. (i.e. the multiplication of |sizeof(uint8_t)|)

@@ +609,5 @@
> +    static_cast<uint16_t>(aManufacturerLen * sizeof(uint8_t)),
> +    static_cast<uint16_t>(aServiceDataLen * sizeof(uint8_t)), /* Byte Length */
> +    static_cast<uint16_t>(aServiceUuidsLen * sizeof(BluetoothUuid)),
> +    PackArray<uint8_t>(aManufacturerData, aManufacturerLen),
> +    PackArray<uint8_t>(aServiceData, aServiceDataLen),

ditto

::: dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.h
@@ +212,1 @@
>                                 BluetoothGattResultHandler* aRes);

Suggest to simplify the interface of |BluetoothDaemonGattModule::ClientSetAdvDataCmd| by using the similar prototype of |BluetoothDaemonGattInterface::SetAdvData| to hide details of the backend as much as possible.

::: dom/bluetooth/common/BluetoothCommon.cpp
@@ +34,5 @@
> +{
> +    for (size_t i = 0; i < aData.mServiceUuids.Length(); i++) {
> +      BluetoothUuid uuid;
> +      if (NS_WARN_IF(NS_FAILED(StringToUuid(aData.mServiceUuids[i], uuid)))) {
> +        continue;

There should be an error if the conversion fails.

::: dom/bluetooth/common/BluetoothCommon.h
@@ +8,5 @@
>  #define mozilla_dom_bluetooth_BluetoothCommon_h
>  
>  #include <algorithm>
>  #include "mozilla/Compiler.h"
> +#include "mozilla/dom/BluetoothAdapterBinding.h"

I am not sure it is proper to handle DOM binding stuffs in |BluetoothCommon.h|. Since |BluetoothCommon.h| is used extensively in our codes including our backend glues, I would suggest to keep |BluetoothCommon.*| away from DOM binding stuffs to stay neutral. Converting DOM stuffs in utilities or in webidl parts (ex. BluetoothAdapter.cpp or BluetoothGattServer.cpp) would be preferred.

@@ +1261,5 @@
>    GAP_SHORTENED_NAME     = 0X08, // Shortened Local Name
>    GAP_COMPLETE_NAME      = 0X09, // Complete Local Name
>  };
>  
> +struct BluetoothGattAdvertisingData {

It is not clear how these members should be filled and how these members would effect the advertising behavior. Suggest to add comments for each member to express its usage.

@@ +1271,5 @@
> +  nsTArray<BluetoothUuid> mServiceUuids;
> +
> +  BluetoothGattAdvertisingData()
> +    : mIncludeDevName(false)
> +    , mIncludeTxPower(false)

Initialize |mAppearance| as well.

@@ +1275,5 @@
> +    , mIncludeTxPower(false)
> +  { }
> +
> +  BluetoothGattAdvertisingData(
> +    const mozilla::dom::BluetoothAdvertisingData& aData);

Suggest not to handle DOM binding stuffs here.

::: ipc/hal/DaemonSocketPDUHelpers.h
@@ +325,5 @@
>    return NS_OK;
>  }
>  
> +/* |PackArray<PackReversed<U>>| is a helper for packing data of each element in
> + * tje reversed order. Pass an instance of this structure as the first argument

typo?

@@ +327,5 @@
>  
> +/* |PackArray<PackReversed<U>>| is a helper for packing data of each element in
> + * tje reversed order. Pass an instance of this structure as the first argument
> + * to |PackPDU| to pack data of each array element in the reversed order.
> + */

The comment is correct, but it would be good to say something more about the subtle difference of |PackReversed<PackArray<U>>|. How about saying we use |PackReversed<U>| to pack each element of the array?

@@ +340,5 @@
> +  const U* mData;
> +  size_t mLength;
> +};
> +
> +/* This implementation of |PackPDU| packs data in each element in |PackArray|

data of each element
Attachment #8700537 - Flags: review?(brsun)
(In reply to Bruce Sun [:brsun] from comment #6)
> Comment on attachment 8700537 [details] [diff] [review]
> Bug 1228546 - Implement peripheral mode support for GATT API.
> 
> Review of attachment 8700537 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some suggestions about the interface and the behavior:
>  - It might not be so intuitive to imply listening for incoming connection
> in |startAdvertising|. Moreover, |startAdvertising| always allows the device
> to be connectible, which might surprise users if they don't want to make
> their device connectible. Suggest to use a separated API for listening, and
> don't automatically make the device connectible by simply calling
> |startAdvertising|.

From the spec, the device is connectable if it is broadcasting connectable advertising events.
And in 4.2.2 LE Procedures, there is a advertising procedure but no listen procedure.
To me, advertising is a term more align with spec, I think using advertising might be more reasonable here.
Defiantly would add comments to state the connectable fact in webidl for this API. 

BTW, currently it looks like we don't have a way to configure whether we want to broadcast connectable or unconnectable adv events yet.
I think it's OK since the only mandatory adv events a peripheral need to support is Connectable undirected event.

> ::: dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.cpp
> @@ +605,5 @@
> >      PackConversion<bool, uint8_t>(aIsTxPowerIncluded),
> >      PackConversion<int, int32_t>(aMinInterval),
> >      PackConversion<int, int32_t>(aMaxInterval),
> >      PackConversion<int, int32_t>(aApperance),
> > +    static_cast<uint16_t>(aManufacturerLen * sizeof(uint8_t)),
> 
> Not sure about the intention of |sizeof(uint8_t)|. Maybe using |elem_type|
> of the instance of |nsTArray<T>|?
> 

The intention is to calculate the byte length that daemon need to read off, just like
https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.cpp#2302
https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.cpp#2336
https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.cpp#2626

> @@ +608,5 @@
> >      PackConversion<int, int32_t>(aApperance),
> > +    static_cast<uint16_t>(aManufacturerLen * sizeof(uint8_t)),
> > +    static_cast<uint16_t>(aServiceDataLen * sizeof(uint8_t)), /* Byte Length */
> > +    static_cast<uint16_t>(aServiceUuidsLen * sizeof(BluetoothUuid)),
> > +    PackArray<uint8_t>(aManufacturerData, aManufacturerLen),
> 
> The length calculation is not sync with other parameter of the function.
> (i.e. the multiplication of |sizeof(uint8_t)|)
> 

The second parameter for PackArray is the number of elements.

> ::: dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.h
> @@ +212,1 @@
> >                                 BluetoothGattResultHandler* aRes);
> 
> Suggest to simplify the interface of
> |BluetoothDaemonGattModule::ClientSetAdvDataCmd| by using the similar
> prototype of |BluetoothDaemonGattInterface::SetAdvData| to hide details of
> the backend as much as possible.
> 

This simply follows what |DaemonGattInterface::WriteChar/Desc| does.
I'm not sure what the backend details you referred to here?
> > +struct BluetoothGattAdvertisingData {
> 
> It is not clear how these members should be filled and how these members
> would effect the advertising behavior. Suggest to add comments for each
> member to express its usage.
> 
How about we add into our wiki document instead of here?
This structure is just mapping every attribute in BluetoothAdvertisingData used in our webapi with proper data types for backend usage, maybe the api document is needed instead of adding comments here?
> This simply follows what |DaemonGattInterface::WriteChar/Desc| does.
> I'm not sure what the backend details you referred to here?

I'm just wondering why the prototype of |BluetoothDaemonGattModule::ClientSetAdvDataCmd| and |BluetoothDaemonGattInterface::SetAdvData| are so different. My naive guess of the reason is to try to simulate the backend interface. Please help correct me if it is wrong.

Would you share the idea why not using similar prototypes for |BluetoothDaemonGattClientInterface::WriteCharacteristic| and |BluetoothDaemonGattModule::ClientWriteCharacteristicCmd|?

> > It is not clear how these members should be filled and how these members
> > would effect the advertising behavior. Suggest to add comments for each
> > member to express its usage.
> > 
> How about we add into our wiki document instead of here?
> This structure is just mapping every attribute in BluetoothAdvertisingData
> used in our webapi with proper data types for backend usage, maybe the api
> document is needed instead of adding comments here?

Our wiki definitely have to document the usage of these members; our codes have to document them, too. It would be good if maintainers can have a clear idea about the intention of our codes by simply searching comments around those definitions. Otherwise it would be difficult to maintain this structure.

For example, I don't exactly know what these members are for, and what is the relationship between |BluetoothAdvertisingData.manufacturerData| and |BluetoothAdvertisingData.serviceData| and |BluetoothLeDeviceEvent.scanRecord|. Please kindly help to share these information as well. Thanks.
> > ::: dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.cpp
> > @@ +605,5 @@
> > >      PackConversion<bool, uint8_t>(aIsTxPowerIncluded),
> > >      PackConversion<int, int32_t>(aMinInterval),
> > >      PackConversion<int, int32_t>(aMaxInterval),
> > >      PackConversion<int, int32_t>(aApperance),
> > > +    static_cast<uint16_t>(aManufacturerLen * sizeof(uint8_t)),
> > 
> > Not sure about the intention of |sizeof(uint8_t)|. Maybe using |elem_type|
> > of the instance of |nsTArray<T>|?
> > 
> 
> The intention is to calculate the byte length that daemon need to read off,
> just like
> https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/
> BluetoothDaemonGattInterface.cpp#2302
> https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/
> BluetoothDaemonGattInterface.cpp#2336
> https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/
> BluetoothDaemonGattInterface.cpp#2626
> 
It might be hard to read using elem_type, for example, it will be 'sizeof(RemoveReference<decltype(aManufacturerData)>::Type::elem_type)' for manufacturerData.
Personally I would prefer just write uint8_t since we could easily see the element type of this array from parameter list.
- address above review comments.
Attachment #8700537 - Attachment is obsolete: true
Attachment #8703519 - Flags: review?(brsun)
Blocks: 1210724
Comment on attachment 8703519 [details] [diff] [review]
Bug 1228546 (v2) - Implement peripheral mode support for GATT API.

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

Almost there. Please kindly check comments inline.

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +988,5 @@
> +  ENSURE_GATT_INTF_IS_READY_VOID(aRunnable);
> +
> +  size_t index = sClients->IndexOf(aAppUuid, 0 /* Start */, UuidComparator());
> +
> +  // Reject the startAdvertising request if the clientIf is being used.

Just curious about the reason why we reject the start advertising request for existing clintIf is due to the limitation of the backend or due to our design. It might be worth to leave comments for |aAppUuid| around the declaration of |StartAdvertising| if |aAppUuid| passed to |StartAdvertising| should be a brand new UUID instead of a registered UUID.

@@ +1866,5 @@
> +  }
> +  RefPtr<BluetoothGattServer> server = (*sServers)[index];
> +
> +  if (server->mServerIf > 0) {
> +    DispatchReplySuccess(aRunnable);

Suggest to add comments to clarify the behavior how |RegisterServer| handles in-coming runnables if the server is being registered and if the server has been registered.

@@ +1870,5 @@
> +    DispatchReplySuccess(aRunnable);
> +  } else if (!server->mIsRegistering) { /* avoid triggering another registration
> +                                         * procedure if there is an on-going one
> +                                         * already */
> +    server->mRegisterServerRunnable = aRunnable;

|server->mRegisterServerRunnable| should be checked first before assigning to a new value.

@@ +2784,5 @@
>        DispatchReplyError(client->mConnectRunnable,
>                           NS_LITERAL_STRING(
>                             "Connect failed due to registration failed"));
>        client->mConnectRunnable = nullptr;
> +    } else if (client->mStartAdvertisingRunnable) {

|client->mStartAdvertisingRunnable| should be checked in an individual |if| block instead of in a |else if| block.

@@ +2817,5 @@
>      sBluetoothGattInterface->Connect(
>        aClientIf, client->mDeviceAddr, true /* direct connect */,
>        TRANSPORT_AUTO,
>        new ConnectResultHandler(client));
> +  } else if (client->mStartAdvertisingRunnable) {

Ditto.

::: dom/bluetooth/common/webapi/BluetoothAdapter.cpp
@@ +32,5 @@
>  #include "mozilla/dom/File.h"
>  
>  #include "mozilla/dom/bluetooth/BluetoothAdapter.h"
>  #include "mozilla/dom/bluetooth/BluetoothClassOfDevice.h"
> +#include "mozilla/dom/bluetooth/BluetoothCommon.h"

This file probably has been included in |BluetoothAdapter.h| already. Would you mind helping me double check it again?

::: dom/bluetooth/common/webapi/BluetoothGattServer.cpp
@@ +414,5 @@
> +    }
> +
> +    bs->StartAdvertisingInternal(
> +      mServer->mAdvertisingAppUuid, mAdvData,
> +      new BluetoothVoidReplyRunnable(nullptr, mPromise));

|mAdvertisingAppUuid| would need to be cleared if advertising fails.

@@ +419,5 @@
> +  }
> +
> +  virtual void OnErrorFired() override
> +  {
> +    mPromise->MaybeReject(NS_ERROR_DOM_OPERATION_ERR);

|mAdvertisingAppUuid| would need to be cleared if registration fails.

@@ +458,5 @@
> +  BluetoothGattAdvertisingData data;
> +  rv = AdvertisingDataToGattAdvertisingData(aAdvData, data);
> +  BT_ENSURE_TRUE_REJECT(NS_SUCCEEDED(rv), promise, rv);
> +
> +  if (mServerIf > 0) {

|BluetoothGattServer::mServerIf| is not suitable for real time checking whether the server has been registered or not.

Since |BluetoothGattManager::RegisterServer| can handle multiple registration with the same application UUID with a success return value, suggest always using |RegisterServerAndStartAdvertisingTask| to start advertising.

@@ +504,5 @@
> +    mGattServer = nullptr;
> +  }
> +
> +private:
> +  RefPtr<BluetoothGattServer> mGattServer;

Would you mind syncing the naming of |RefPtr<BluetoothGattServer>| member variable in |RegisterServerAndStartAdvertisingTask| and |StopAdvertisingTask|?

::: dom/bluetooth/common/webapi/BluetoothGattServer.h
@@ +184,5 @@
> +
> +  /**
> +   * AppUuid of the GATT client interface which is used to advertise.
> +   */
> +  BluetoothUuid mAdvertisingAppUuid;

Could we reuse |mAppUuid| for the advertising? Just curious.

::: dom/webidl/BluetoothGattServer.webidl
@@ +29,5 @@
> +   * The first 2 octets contain the Company Identifier Code followed by
> +   * additional manufacturer specific data. Please see Core Specification
> +   * Supplement (CSS) v6 1.4 for more details.
> +   */
> +  ArrayBuffer? manufacturerData = null;

How about using an individual value to store Company Identifier? So that app developers don't need to care about the exact position of the identifier in the byte array, and they don't need to concatenate these 2 octets into the array buffer by themselves.

@@ +35,5 @@
> +  /**
> +   * Consists of a service UUID with the data associated with that service.
> +   * Please see Core Specification Supplement (CSS) v6 1.11 for more details.
> +   */
> +  ArrayBuffer? serviceData = null;

Ditto. How about using an individual values to store the service UUID? So that app developers don't need to care about the conversion between UUID in DOMString and UUID in bytes. It would be good if we could hide endian burdens for them.
Attachment #8703519 - Flags: review?(brsun)
Hi Jocelyn,

Sorry for my lack of experiences. Would you mind sharing how |BluetoothLeDeviceEvent.scanRecord| could reflect the manufacturer data and the service data from the remote peripheral server?
Please see my comments inline, thanks!
(In reply to Bruce Sun [:brsun] from comment #12)
> Comment on attachment 8703519 [details] [diff] [review]
> Bug 1228546 (v2) - Implement peripheral mode support for GATT API.
> 
> Review of attachment 8703519 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Almost there. Please kindly check comments inline.
> 
> ::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
> @@ +988,5 @@
> > +  ENSURE_GATT_INTF_IS_READY_VOID(aRunnable);
> > +
> > +  size_t index = sClients->IndexOf(aAppUuid, 0 /* Start */, UuidComparator());
> > +
> > +  // Reject the startAdvertising request if the clientIf is being used.
> 
> Just curious about the reason why we reject the start advertising request
> for existing clintIf is due to the limitation of the backend or due to our
> design. It might be worth to leave comments for |aAppUuid| around the
> declaration of |StartAdvertising| if |aAppUuid| passed to |StartAdvertising|
> should be a brand new UUID instead of a registered UUID.
> 

It's due to our design.
Currently the clientIf is registered when startAdvertising using newly generated UUID and unregistered when stopAdvertising.
So we didn't expect a registered UUID.

The life cycle of the clientIf is designed as this way mainly because I don't want to expose too much client stuff in BluetoothGattServer.

> ::: dom/bluetooth/common/webapi/BluetoothGattServer.h
> @@ +184,5 @@
> > +
> > +  /**
> > +   * AppUuid of the GATT client interface which is used to advertise.
> > +   */
> > +  BluetoothUuid mAdvertisingAppUuid;
> 
> Could we reuse |mAppUuid| for the advertising? Just curious.
> 

Yes I think we could.
Just personally prefer to using a complete new one in each iteration to make the logic simpler, as same as LeScan.
Would you prefer to reuse it?
(In reply to Bruce Sun [:brsun] from comment #13)
> Hi Jocelyn,
> 
> Sorry for my lack of experiences. Would you mind sharing how
> |BluetoothLeDeviceEvent.scanRecord| could reflect the manufacturer data and
> the service data from the remote peripheral server?

scanRecord is consist of multiple AD structures (type + data).
From setting this two data, scanRecord would have two AD structures in its scanRecord, [manufacturerData AD type + data] and [service data AD type + data].
Comment on attachment 8705001 [details] [diff] [review]
Bug 1228546 (v3) - Implement peripheral mode support for GATT API.

- revise the logic of runnable handling in |RegisterServer|
- add manufacturerId and serviceUuid to make web API more friendly
- add StartAdvertisingTask to clear UUID on error
- always execute RegisterServerAndStartAdvertisingTask in |StartAdvertising|
Attachment #8705001 - Flags: review?(brsun)
> Yes I think we could.
> Just personally prefer to using a complete new one in each iteration to make
> the logic simpler, as same as LeScan.
> Would you prefer to reuse it?

Either ways are okay for me. But for the sake of maintenance, it would be good to have a obvious note to describe that the passed AppUuid should be generated each time while using the corresponding API.
Comment on attachment 8705001 [details] [diff] [review]
Bug 1228546 (v3) - Implement peripheral mode support for GATT API.

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

Looks good. Please refer to some nits inline.

::: dom/bluetooth/common/BluetoothUtils.cpp
@@ +432,5 @@
> +
> +  if (!aAdvData.mManufacturerData.IsNull()) {
> +    // First two bytes are manufacturer ID in little-endian.
> +    aGattAdvData.mManufacturerData[0] = aAdvData.mManufacturerId;
> +    aGattAdvData.mManufacturerData[1] = aAdvData.mManufacturerId >> 8;

Suggest to use existing endian utilities[1] to handle the conversion.

[1] https://dxr.mozilla.org/mozilla-central/source/mfbt/Endian.h

::: dom/webidl/BluetoothGattServer.webidl
@@ +26,5 @@
> +  /**
> +   * Company Identifier Code for manufacturer data. Please see Core
> +   * Specification Supplement (CSS) v6 1.4 for more details.
> +   */
> +  unsigned short manufacturerId = 0;

|manufacturerId| is a mandatory field by simply glancing the WebIDL. But whether it will be actually used is depended on the value of |manufacturerData| (based on the implementation in the patch).

Suggest to add a description to address the relationship of these two fields.

@@ +35,5 @@
> +   */
> +  ArrayBuffer? manufacturerData = null;
> +
> +  /**
> +   * 128 bit Service UUID for service data. Please see Core Specification

nit: 128-bit or 128 bits?

@@ +38,5 @@
> +  /**
> +   * 128 bit Service UUID for service data. Please see Core Specification
> +   * Supplement (CSS) v6 1.11 for more details.
> +   */
> +  DOMString serviceUuid = "";

Ditto. The same suggestion for |manufacturerId| are applicable to |serviceUuid| as well.
Attachment #8705001 - Flags: review?(brsun) → review+
Hi Blake,

This bug added several API to enable us to act as a BLE peripheral which could broadcast advertisements to nearby devices and accept their connection requests.
Could you help to review it from the DOM perspective?

Patch Summary:
1. BluetoothGattServer.webidl: 
  startAdvertising/stopAdvertising methods and BluetoothAdvertisingData dictionary.
2. BluetoothGattServer.cpp/h: Implementation of start/stopAdvertising methods
3. BluetoothDaemonGattInterface.cpp/h, BluetoothGattManager.cpp: Interact with bluetooth backend
Others: ipc stuff and daemon packet handling

Thanks,
Jocelyn
Attachment #8705001 - Attachment is obsolete: true
Attachment #8706766 - Flags: review?(mrbkap)
Attachment #8706766 - Flags: review?(mrbkap) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2ebac854c7fa
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Change the dependence.
Blocks: 933358
No longer blocks: 1210724
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: