-
Notifications
You must be signed in to change notification settings - Fork 85
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
Enhanced Scan Filtering #695
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.
Pass 1/x
Overall, I love this PR! Amazing work!
I created a branch with some small changes (twyatt/FilterPredicateSet
), let me know what you think.
I'm using a typealias
but I think just using the collection type directly would work.
If you wanted type safety not offered by the collection type, then maybe a value class
would be a better choice than data class
?
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.
Did some basic testing using SensorTag sample app (Android and Apple) and everything worked great!
Love all the unit tests! Awesome work!
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.
Sorry I got to this late, this is awesome.
Closes #586
Possibly relates to #627?
After noodling around on the
and/or
thing, it was found a mini-DSL/builder style to be simpler to implement, and also prevents the ability to specify non-sensical or conflicting filters.Description
Everything in a
match
block is a single "predicate" which must all match to satisfy the condition (aka ANDed together). And any of thematch
predicate blocks can match to satisfy the overall set of filters (aka ORed together). The model of the composed logic is like this:Example, to match on all devices with a certain name:
To match on a name AND a service uuid:
To match on a name OR a service uuid:
System Filters
The native platforms provide an option to inject filters into the system scanner so that BLE radio usage can be optimized. This new filter model can be transcribed to match the system filter shape in many cases, so the preference is to convert it and inject it when possible, and fallback to a broader scan with filtering applied on the consumer side if unable to convert.
Web
Injection of the filter into the system "scanner" (aka picker) is the only option. For Web, we are always able to convert as it uses the same general shape and the exact same predicate logic.
Apple
For Apple the system only wants service uuids and the predicate logic takes the form of "uuid-1 OR uuid-2 ... OR uuid-n". So the approach here is to inject all uuid filters, but only if every clause has at least one uuid. This narrows the incoming advertisements to only those of interest, but we still must apply the provided filters to execute the full logic of the requested scan against each received advertisement.
Android
The Android system scanner supports matching on name, address, uuid and manufacturer data (and a whole lot more that we do not yet support). But the limitations are that each filter accepts only a single uuid or manufacturer data, and it does not support name prefix matching. We could get fancy and combinatorially unroll any compound uuid or manufacturer data clauses to maximize ability to enable the system filter, leaving only name-prefix as the exception. But, for now at least, decided on the simpler approach of detecting a direct translation fit, and falling back to a "scan everything and post-filter" if there are any name-prefix or compound clauses that do not directly convert.
Breaking Changes
As a breaking change it was decided to replace
Filter.Name
andFilter.NamePrefix
with thesealed class Name { Exact; Prefix }
. Having these separated breaks the logical operations becauseName
andNamePrefix
are mutually exclusive, so are best expressed as a union/sum type. Currently users may specify both aName
and aNamePrefix
which makes no real sense. (it is ok to OR them but not to AND them)Also removed all filter related API that was marked as deprecated with level ERROR.