-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Allow using allowlist for BLE scanner #6752
base: dev
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #6752 +/- ##
==========================================
+ Coverage 53.70% 54.09% +0.38%
==========================================
Files 50 50
Lines 9408 9627 +219
Branches 1654 1701 +47
==========================================
+ Hits 5053 5208 +155
- Misses 4056 4092 +36
- Partials 299 327 +28 ☔ View full report in Codecov by Sentry. |
@@ -213,6 +214,15 @@ class ESP32BLETracker : public Component, | |||
void gap_event_handler(esp_gap_ble_cb_event_t event, esp_ble_gap_cb_param_t *param) override; | |||
void ble_before_disabled_event_handler() override; | |||
|
|||
static void uint64_to_bd_addr(uint64_t address, esp_bd_addr_t bd_addr) { |
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.
We should avoid duplicating the code since this function already exists in bluetooth_proxy.h
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.
ya...I was trying tot think of the best approach for that. Can I easily pull in the proxy one, or should the proxy one switch to referencing this one? The latter seems better as the proxy requires this component anyway.
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.
since proxy depends on esp32_ble_tracker it probably makes sense to move it here.
I'd do that in another PR and than rebase this one on top once its merged
9a098fb
to
a36b8fd
Compare
@@ -30,6 +30,9 @@ | |||
CONF_WINDOW = "window" | |||
CONF_CONTINUOUS = "continuous" | |||
CONF_ON_SCAN_END = "on_scan_end" | |||
CONF_ALLOWLIST_ADDRESS = "allowlist_address" | |||
MAX_ALLOWLIST = 10 |
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.
The max depends on which board is being used, so I picked what should be a more conservative amount.
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.
The max allowlist is hardcoded? How does the user know about this? Any way to change this value?
Eg. Using 12 Xiaomi temp/hum sensors and a BLE lock...
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.
@nagyrobi thus far this is the only spot where I've read anything conclusive: espressif/esp-idf#9205
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 use the max possible for every chip type, or at least make it configurable.
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 disagree, but I don't have the time to figure out what's possible for every chip (or even know where to look). Could be a good follow-up PR. I feel like this at least adds a beneficial feature, and the risk of making it configurable is that if an incorrect value is used by the end-user it won't work properly, so this feels like safer approach to me.
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.
Why does this feature have to depend on IDF?
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.
@nagyrobi the methods used are for the ESP-IDF API. Arduino may have similar functionality but I can't find much info on it.
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 expect they are also available on Arduino. Switching the framework might be all that is needed to verify
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.
The esp-idf apis are all available even when using Arduino (version dependent for some things)
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'll give Arduino a try when I get a moment and adjust as necessary
esp_err_t err = esp_ble_gap_set_scan_params(&this->scan_params_); | ||
if (err != ESP_OK) { | ||
ESP_LOGE(TAG, "esp_ble_gap_set_scan_params failed: %d", err); | ||
return; | ||
} | ||
|
||
if (!this->allowlist_address_vec_.empty() && !this->allowlist_populated_) { |
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 wanted to run this in setup()
as it really only needs to run the first time...but then it wasn't working at all, so I'm just using a flag to save it from running on successive passes.
ca9532d
to
4921b8c
Compare
@bdraco docs changes are up, should be ready for a proper review now. |
What does this implement/fix?
Enables using the esp-idf BLE scanner's whitelist functionality. This is useful in cases where there's a lot of BLE devices nearby and they overwhelm the scanner, especially in
active
mode.My own example is using this along with the Bluetooth Proxy component to enable the Home Assistant Yale Bluetooth integration. I have 3 locks, 2 close to one proxy, one close to another, but the extra BLE traffic can cause significant delays. I have no other BLE devices that I'm integrating at this point, so filtering down to those devices is very beneficial.
Types of changes
Related issue or feature (if applicable): fixes
I think this would basically complete esphome/feature-requests#2686 as well (the ask for was for the proxy, but it relies on this component for scanning anyway)
Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#3848
Test Environment
Example entry for
config.yaml
:Checklist:
tests/
folder).If user exposed functionality or configuration variables are added/changed: