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

Accept filters on manufacturer and service data. #297

Merged
merged 5 commits into from
Sep 27, 2016

Conversation

Copy link
Contributor

@g-ortuno g-ortuno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it looks pretty good overall. I just had a two questions.

[{manufacturerId: 17},
{serviceDataUUID: A}]
[{manufacturerData: {17: {}}},
{serviceData: {"A": {}}}]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a variant that has both in the same filter and no device is found?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, done.

@@ -529,16 +533,22 @@ spec: html
<h2 id="device-discovery" oldids="requestDevice-secure-context">Device Discovery</h2>

<pre class="idl">
dictionary BluetoothRequestDeviceFilter {
dictionary BluetoothDataFilterInit {
BufferSource dataPrefix;
Copy link
Contributor

@g-ortuno g-ortuno Sep 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a silly question but what can you do with dataPrefix that you can't with just a mask? Is it that dataPrefix: [0x1] would match both service data: [0x1] and [0x3] but mask would only match the former?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to match [0x1] and [0x3] but not [0x2], you'd use {dataPrefix: [0x1], mask: [0xfd]} to say that the lowest bit has to be 1, the second bit doesn't matter, and the rest have to be 0. There's no way to say that without using both dataPrefix and mask.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation!

<td>
<pre highlight="js">
requestDevice({
filters:[{manufacturerData: {}}]
Copy link
Contributor

@g-ortuno g-ortuno Sep 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to have this match devices that advertise any manufacturer data? Similarly for namePrefix: would it make sense for an empty string to match any device that advertises a name?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense, but it's less power-efficient to match everything, so I want to discourage developers from doing it accidentally. The spec currently does that by making them use the separate acceptAllAdvertisements field.

I might be overthinking that, and the spec would get simpler if I gave up on trying to enforce power-efficiency here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I bias slightly for acceptAllAdvertisements - not making it 'easy' to wildcard.

@@ -529,16 +533,22 @@ spec: html
<h2 id="device-discovery" oldids="requestDevice-secure-context">Device Discovery</h2>

<pre class="idl">
dictionary BluetoothRequestDeviceFilter {
dictionary BluetoothDataFilterInit {
BufferSource dataPrefix;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these take BufferSources (ArrayBuffers and views) or sequences of bytes? Sequences of bytes will be more convenient to write out at the call site, but maybe harder to assemble programmatically if there are multi-byte fields in the data.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion either way but I think it might be a bit strange to use BufferSources every where but in one place. Though I agree that it would make calls easier.

};

dictionary RequestDeviceOptions {
sequence&lt;BluetoothRequestDeviceFilter> filters;
sequence&lt;BluetoothLEScanFilterInit> filters;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's OK, but as a reader I didn't follow why these are FooTypeInit types instead of just FooType

Copy link
Member Author

@jyasskin jyasskin Sep 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're FooInit because there's a Foo over in the Scanning spec. I could give them different names here, and inherit from them in the Scanning spec, or I could move the Scanning interfaces over here even though they're unused in this spec. None of the options look particularly good.

<td>
<pre highlight="js">
requestDevice({
filters:[{manufacturerData: {}}]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I bias slightly for acceptAllAdvertisements - not making it 'easy' to wildcard.

filters: [{manufacturerId: 0x004C}],
filters: [{manufacturerData: {0x004C: {dataPrefix: new Uint8Array([
0x02, 0x15, // iBeacon identifier.
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15 // My beacon UUID.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

};

dictionary RequestDeviceOptions {
sequence&lt;BluetoothRequestDeviceFilter> filters;
sequence&lt;BluetoothLEScanFilterInit> filters;
Copy link
Member Author

@jyasskin jyasskin Sep 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're FooInit because there's a Foo over in the Scanning spec. I could give them different names here, and inherit from them in the Scanning spec, or I could move the Scanning interfaces over here even though they're unused in this spec. None of the options look particularly good.

type: dfn
text: a copy of the bytes held; url: dfn-get-buffer-source-copy

spec: web-bluetooth; urlPrefix: index.html#
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are in anchors here because we have to commit this PR for Shepherd to pick up the definitions in it.

@jyasskin jyasskin force-pushed the masked-data-prefixes branch from 39fe16c to 22cebd7 Compare September 24, 2016 01:50
Copy link
Contributor

@g-ortuno g-ortuno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@jyasskin jyasskin force-pushed the masked-data-prefixes branch 3 times, most recently from 22cebd7 to 9da334a Compare September 26, 2016 21:26
@jyasskin jyasskin force-pushed the masked-data-prefixes branch from 321d215 to 6a90364 Compare September 27, 2016 21:31
@jyasskin jyasskin merged commit 36a5e51 into WebBluetoothCG:master Sep 27, 2016
@jyasskin jyasskin deleted the masked-data-prefixes branch September 27, 2016 21:36
@irgu
Copy link

irgu commented Sep 28, 2016

At the beginning - thanks for decision that this feature will be included in specification. My only suggestion is that 'mask/prefix' is not so intuitive to use and something like 'pattern/ignore' would be little better and more flexible. I'm not JavaScript expert, so allowed myself to write small example in C to illustrate this:

#include <stddef.h>
#include <stdbool.h>
#include <stdio.h>

char service_data_1[] = {0xAA, 0xB3, 0xCC}; //10101010 10110011 11001100
char service_data_2[] = {0xAE, 0x8B, 0xCD}; //10101110 10001011 11001101

char pattern[]        = {0xA0, 0x00, 0xCC}; //10100000 00000000 11001100
char ignore[]         = {0x0F, 0xFF, 0x00}; //00001111 11111111 00000000

bool service_data_check(char * p_service_data, size_t service_data_size, char * p_pattern, char * p_ignore)
{
    for(char i=0; i<service_data_size; i++)
    {
        if( (p_service_data[i] & (~p_ignore[i])) == (p_pattern[i] & (~p_ignore[i])) )
        {
            //byte matches - go ahead
        }
        else
        {
            //byte not matches - stop checking
            printf("Service data doesn't match the pattern.\n");
            return false;
        }
    }

    printf("Service data match the pattern.\n");
    return true;
}

int main()
{
    service_data_check(service_data_1, sizeof(service_data_1), pattern, ignore);
    service_data_check(service_data_2, sizeof(service_data_2), pattern, ignore);

    return 0;
}

@jyasskin
Copy link
Member Author

jyasskin commented Sep 28, 2016

@irgu I think your proposal is to rename dataPrefix to pattern and to invert mask and rename it to ignore. Is that right? I'm not sure what behavior you want in the case that the advertised service data is longer than the pattern, since your C code has undefined behavior in that case.

I'm definitely inclined to keep "prefix" in the name if the behavior is to assert a prefix. "Mask" vs "ignore" is a matter of taste; I've followed Android's API here. Are there other Bluetooth APIs that use "ignore"?

@irgu
Copy link

irgu commented Sep 29, 2016

@jyasskin I looked at it once again and I totally agree with you. Current solution has the same flexibility and 'prefix' is reasonable name. It was just a small incomprehension of naming scheme when I saw this for the first time. Thanks for clearing :)

@beaufortfrancois
Copy link
Member

FYI I'm thinking of using sequence instead of map for those filters. See #545

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for requesting devices with specific service data in adv packets
5 participants