Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make requestDevice()'s filters match requestLEScan()'s filters. #265

Conversation

@@ -25,6 +25,7 @@ GATT Server Disconnect | ✓ | ✓ | ✓ | ✓ |
Get Characteristics List | ✓ | ✓ | 53 | ✓ | |
Device Disconnected Event | ✓ | ✓ | ✓ | ✓ | |
Get Primary Services List | 53 | 53 | 53 | 53 | |
Manufacturer/Service data filters | | | | | |

This comment has been minimized.

Copy link
@beaufortfrancois

beaufortfrancois Aug 4, 2016

Member

Could we add it below └ Name or prefix second row as └ Manufacturer/Service data to make it clear it is a filter?

This comment has been minimized.

Copy link
@jyasskin

jyasskin Aug 4, 2016

Author Member

Thanks, done.

@beaufortfrancois

This comment has been minimized.

Copy link
Member

beaufortfrancois commented Aug 4, 2016

LGTM but nit

@jyasskin jyasskin force-pushed the jyasskin:match-requestDevice-filters-to-scanning branch from d9a2221 to e921f54 Aug 4, 2016
@g-ortuno

This comment has been minimized.

Copy link
Contributor

g-ortuno commented Aug 5, 2016

qq: Are you thinking of changing the current filters to match the masking behavior on Android? The Eddystone EID seems like a good use case for it as mentioned by @jfarfel.

@jyasskin

This comment has been minimized.

Copy link
Member Author

jyasskin commented Aug 5, 2016

Yes, but I'm not yet sure how to spell it in Javascript, or whether the length restriction in the Android API is exactly what we want.

@jyasskin jyasskin force-pushed the jyasskin:match-requestDevice-filters-to-scanning branch from e921f54 to c52ac03 Aug 5, 2016
@g-ortuno

This comment has been minimized.

Copy link
Contributor

g-ortuno commented Aug 5, 2016

The length? You mean the scanning duration restriction?

Also are you planning on including that change in this CL? Just wanted to know if I should review this now.

@jyasskin

This comment has been minimized.

Copy link
Member Author

jyasskin commented Aug 5, 2016

Sorry, I meant that [ScanFilter.Builder.setManufacturerData(id, data, mask)](https://developer.android.com/reference/android/bluetooth/le/ScanFilter.Builder.html#setManufacturerData%28int, byte[], byte[]%29) requires that the advertised data be exactly as long as the array you provide, rather than letting it be a prefix or suffix. For example, you can't scan for Eddystone URL packets using the Android filter format without also getting all other Eddystone types.

That change won't be in this PR. I'll figure it out and change both scanning.bs and index.bs in some other PR.

@jfarfel

This comment has been minimized.

Copy link

jfarfel commented Aug 5, 2016

Sorry, I meant that ScanFilter.Builder.setManufacturerData(id, data, mask)https://developer.android.com/reference/android/bluetooth/le/ScanFilter.Builder.html#setManufacturerData(int,%20byte%5B%5D,%20byte%5B%5D) requires that the advertised data be exactly as long as the array you provide, rather than letting it be a prefix or suffix.

I think the data you provide can be a prefix of the full Mfg Data in the
advertisement.

Also, Eddystone data is in Service Data, not Mfg Data. So you'd want
setServiceData(uuid,
data, mask)
<https://developer.android.com/reference/android/bluetooth/le/ScanFilter.Builder.html#setServiceData(android.os.ParcelUuid,
byte[], byte[])>. But the same thing still applies - you can give a prefix
(but not a suffix). See code:
https://android.googlesource.com/platform/frameworks/base/+/marshmallow-release/core/java/android/bluetooth/le/ScanFilter.java#354

For example, you can't scan for Eddystone URL packets using the Android filter format without also getting all other Eddystone types.

Because it can be a prefix, I believe that you can indeed do this (since
the type is in a consistent spot in the service data).

@jyasskin

This comment has been minimized.

Copy link
Member Author

jyasskin commented Aug 5, 2016

Oh, yep, that'll work. I still want to commit this PR making the two sets of filters the same, but then I'll think about how to spell the masked prefix filter in JS.

@jyasskin jyasskin force-pushed the jyasskin:match-requestDevice-filters-to-scanning branch from c52ac03 to c396169 Aug 8, 2016
@@ -524,6 +524,8 @@ spec: permissions
sequence&lt;BluetoothServiceUUID> services;
DOMString name;
DOMString namePrefix;
unsigned short manufacturerId;

This comment has been minimized.

Copy link
@g-ortuno

g-ortuno Aug 12, 2016

Contributor

Would it make sense to make these two arrays? How else could you express that you want a beacon that is advertising data for two services or two manufacturer's ids?

This comment has been minimized.

Copy link
@beaufortfrancois

beaufortfrancois Aug 12, 2016

Member

Bluetooth Core Spec 4.0 says respectively in Sections 10.18.10 and 10.18.11 that these values are unique.

Value Description Information
0x16 Service Data (2 or more octets) The first 2 octets contain the 16 bit Service UUID followed by additional service data
Value Description Information
0xFF Manufacturer Specific Data (2 or more octets) The first 2 octets contain the Company Identifier Code followed by additional manufacturer specific data

This comment has been minimized.

Copy link
@g-ortuno

g-ortuno Aug 12, 2016

Contributor

What volume and part? I can't find any 10.18 sections?

This comment has been minimized.

Copy link
@jyasskin

jyasskin Aug 12, 2016

Author Member

CSSv6 A.1.4 and A.1.11 don't say that a packet "shall not contain more than one instance", so it's possible we'd see multiples. However, folks seem to be avoiding the 4-byte overhead of switching services or manufacturers by just tacking whatever data they need into a single field, so I suspect we'd see little use from an array of IDs. Also, is it likely that you'd need both IDs in order to identify the class of devices? Wouldn't they generally advertise one thing that's unique to the device and maybe another generic one?

This comment has been minimized.

Copy link
@jyasskin

jyasskin Sep 20, 2016

Author Member

FWIW, I believe BT5 changes the situation by allowing much larger advertising packets, which is likely to increase the frequency that we see multiple different manufacturers' or services' data. I'm now inclined to follow @g-ortuno's suggestion here.

This comment has been minimized.

Copy link
@beaufortfrancois

beaufortfrancois Sep 20, 2016

Member

@jyasskin @g-ortuno I've successfully broadcasted two manufacturer data IDs (one in advertisement packet, one in scan response) and from what I can see BlueZ handles it properly.

Therefore, I was wrong and we should make manufacturerData and serviceData arrays.

btmon logs:

> HCI Event: LE Meta Event (0x3e) plen 16
      LE Advertising Report (0x02)
        Num reports: 1
        Event type: Scan response - SCAN_RSP (0x04)
        Address type: Random (0x01)
        Address: DA:23:48:6B:B1:F5 (Static)
        Data length: 4
        Company: not assigned (1027)
          Data: 
        RSSI: -45 dBm (0xd3)
@ Device Found: DA:23:48:6B:B1:F5 (2) rssi -45 flags 0x0000
        02 01 06 04 09 46 52 00 03 ff 01 02 03 ff 03 04  .....FR.........

bluetoothctl logs:

[bluetooth]# info DA:23:48:6B:B1:F5 
Device DA:23:48:6B:B1:F5
        Name: FR
        Alias: FR
        Paired: no
        Trusted: no
        Blocked: no
        Connected: no
        LegacyPairing: no
        ManufacturerData Key: 0x0201
        ManufacturerData Key: 0x0403 

DBUS logs:

            dict entry(
               string "ManufacturerData"
               variant                   array [
                     dict entry(
                        uint16 513
                        variant                            array [
                           ]
                     )
                     dict entry(
                        uint16 1027
                        variant                            array [
                           ]
                     )
                  ]
            )
if that member is present,
</li>
<li>
advertise <a>manufacturer specific data</a> with

This comment has been minimized.

Copy link
@g-ortuno

g-ortuno Aug 12, 2016

Contributor

Hmm I wonder if this is going to make things confusing when multiple pages start to connect to the same device. For example the quad copter that only advertises manufacturer data. You would have to have a filter for the service uuid in case you are connected and one for the manufacturer data in case you are not connected. But maybe it's just something developers will have to deal with?

This comment has been minimized.

Copy link
@jyasskin

jyasskin Aug 12, 2016

Author Member

I think they'll just have to deal with identifying the device in two different ways if it behaves significantly differently. I've added a paragraph here mentioning that.

</li>
<li>
advertise <a>service data</a> with
a UUID equal to <dfn dict-member for="BluetoothRequestDeviceFilter">serviceDataUUID</dfn>
if that member is present.
</li>
</ul>

This comment has been minimized.

Copy link
@g-ortuno

g-ortuno Aug 12, 2016

Contributor

nit: The line after this says "...the origin is allowed to access to any service".

This comment has been minimized.

Copy link
@jyasskin

jyasskin Aug 12, 2016

Author Member

Fixed.

@g-ortuno

This comment has been minimized.

Copy link
Contributor

g-ortuno commented Aug 12, 2016

Overall it looks good. Just a couple of comments.

@jyasskin jyasskin force-pushed the jyasskin:match-requestDevice-filters-to-scanning branch from c396169 to 5c50c29 Aug 12, 2016
index.bs Outdated
if a device changes its behavior significantly when it connects,
for example by not advertising its identifying manufacturer data anymore
and instead having the client discover some identifying GATT services,
you may need to include filters both both behaviors.

This comment has been minimized.

Copy link
@g-ortuno

g-ortuno Aug 12, 2016

Contributor

s/both both/for both/ ?

This comment has been minimized.

Copy link
@jyasskin

jyasskin Aug 12, 2016

Author Member

I promise I can normally type.

jyasskin added 2 commits Aug 12, 2016
@g-ortuno

This comment has been minimized.

Copy link
Contributor

g-ortuno commented Aug 12, 2016

lgtm

@jyasskin jyasskin merged commit 3ff565c into WebBluetoothCG:master Aug 12, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jyasskin jyasskin deleted the jyasskin:match-requestDevice-filters-to-scanning branch Aug 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.