-
Notifications
You must be signed in to change notification settings - Fork 192
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
Accept filters on manufacturer and service data. #297
Conversation
There was a problem hiding this 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": {}}}] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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: {}}] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these take BufferSource
s (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.
There was a problem hiding this comment.
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 BufferSource
s every where but in one place. Though I agree that it would make calls easier.
}; | ||
|
||
dictionary RequestDeviceOptions { | ||
sequence<BluetoothRequestDeviceFilter> filters; | ||
sequence<BluetoothLEScanFilterInit> filters; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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: {}}] |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
}; | ||
|
||
dictionary RequestDeviceOptions { | ||
sequence<BluetoothRequestDeviceFilter> filters; | ||
sequence<BluetoothLEScanFilterInit> filters; |
There was a problem hiding this comment.
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# |
There was a problem hiding this comment.
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.
39fe16c
to
22cebd7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
22cebd7
to
9da334a
Compare
…ighten restrictions.
321d215
to
6a90364
Compare
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;
} |
@irgu I think your proposal is to rename 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"? |
@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 :) |
FYI I'm thinking of using |
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