Accept filters on manufacturer and service data. by jyasskin · Pull Request #297 · WebBluetoothCG/web-bluetooth

jyasskin · 2016-09-23T01:42:30Z

Previews at https://api.csswg.org/bikeshed/?url=https://raw.githubusercontent.com/jyasskin/web-bluetooth-1/masked-data-prefixes/index.bs and https://api.csswg.org/bikeshed/?url=https://raw.githubusercontent.com/jyasskin/web-bluetooth-1/masked-data-prefixes/scanning.bs.

Fixes #294. @irgu

g-ortuno

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

g-ortuno · 2016-09-23T03:48:10Z

-            [{manufacturerId: 17},
-             {serviceDataUUID: A}]
+            [{manufacturerData: {17: {}}},
+             {serviceData: {"A": {}}}]


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

Sure, done.

g-ortuno · 2016-09-23T04:18:28Z

  <pre class="idl">
-    dictionary BluetoothRequestDeviceFilter {
+    dictionary BluetoothDataFilterInit {
+      BufferSource dataPrefix;


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?

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.

Thanks for the explanation!

g-ortuno · 2016-09-23T04:20:40Z

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


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?

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.

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

jyasskin · 2016-09-23T16:29:58Z

  <pre class="idl">
-    dictionary BluetoothRequestDeviceFilter {
+    dictionary BluetoothDataFilterInit {
+      BufferSource dataPrefix;


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.

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.

scheib · 2016-09-23T22:44:20Z


    dictionary RequestDeviceOptions {
-      sequence&lt;BluetoothRequestDeviceFilter> filters;
+      sequence&lt;BluetoothLEScanFilterInit> filters;


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

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.

scheib · 2016-09-23T22:45:48Z

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


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

scheib · 2016-09-23T22:47:46Z

-          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.


jyasskin · 2016-09-23T23:17:46Z


    dictionary RequestDeviceOptions {
-      sequence&lt;BluetoothRequestDeviceFilter> filters;
+      sequence&lt;BluetoothLEScanFilterInit> filters;


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.

jyasskin · 2016-09-24T00:32:02Z

+    type: dfn
+        text: a copy of the bytes held; url: dfn-get-buffer-source-copy
+
+spec: web-bluetooth; urlPrefix: index.html#


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

g-ortuno

lgtm

…ighten restrictions.

irgu · 2016-09-28T11:30:56Z

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 · 2016-09-28T20:47:07Z

@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 · 2016-09-29T08:13:25Z

@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 · 2021-04-27T13:45:02Z

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

g-ortuno reviewed Sep 23, 2016

View reviewed changes

beaufortfrancois mentioned this pull request Sep 23, 2016

Support for requesting devices with specific service data in adv packets #294

Closed

jyasskin commented Sep 23, 2016

View reviewed changes

scheib approved these changes Sep 23, 2016

View reviewed changes

jyasskin commented Sep 24, 2016

View reviewed changes

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

g-ortuno approved these changes Sep 25, 2016

View reviewed changes

jyasskin force-pushed the masked-data-prefixes branch 3 times, most recently from 22cebd7 to 9da334a Compare September 26, 2016 21:26

jyasskin added 5 commits September 27, 2016 14:30

Accept filters on manufacturer and service data.

e05e90b

Add an example that shows manufacturer- and serviceData together to t…

ed93b2b

…ighten restrictions.

Remove an unused beacon id variable.

1dd5fa0

A couple cleanups.

90eb0fc

Use CreateDataProperty() to fill in canonicalized-filter fields.

6a90364

jyasskin force-pushed the masked-data-prefixes branch from 321d215 to 6a90364 Compare September 27, 2016 21:31

jyasskin merged commit 36a5e51 into WebBluetoothCG:master Sep 27, 2016

jyasskin deleted the masked-data-prefixes branch September 27, 2016 21:36

beaufortfrancois mentioned this pull request Apr 14, 2021

Adding simple manufacturer id filtering support #544

Closed

Uh oh!

Conversation

jyasskin commented Sep 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

g-ortuno left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

g-ortuno Sep 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

g-ortuno Sep 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jyasskin Sep 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jyasskin Sep 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

g-ortuno left a comment

Choose a reason for hiding this comment

Uh oh!

irgu commented Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jyasskin commented Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

irgu commented Sep 29, 2016

Uh oh!

beaufortfrancois commented Apr 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jyasskin commented Sep 23, 2016 •

edited

Loading

g-ortuno Sep 23, 2016 •

edited

Loading

g-ortuno Sep 23, 2016 •

edited

Loading

jyasskin Sep 23, 2016 •

edited

Loading

jyasskin Sep 23, 2016 •

edited

Loading

irgu commented Sep 28, 2016 •

edited

Loading

jyasskin commented Sep 28, 2016 •

edited

Loading