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

Allow using allowlist for BLE scanner #6752

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Allow using allowlist for BLE scanner #6752

wants to merge 2 commits into from

Conversation

feens
Copy link

@feens feens commented May 15, 2024

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

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Other

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

  • ESP32
  • ESP32 IDF
  • ESP8266
  • RP2040
  • BK72xx
  • RTL87xx

Example entry for config.yaml:

# Example config.yaml
substitutions:
  name: olimex-bluetooth-proxy-00886c
  friendly_name: Bluetooth Proxy (POE)
packages:
  esphome.bluetooth-proxy: github://esphome/firmware/bluetooth-proxy/olimex-esp32-poe-iso.yaml@main
esphome:
  name: ${name}
  name_add_mac_suffix: false
  friendly_name: ${friendly_name}
api:

esp32:
  board: esp32-poe-iso
  framework:
    type: esp-idf
    version: 4.4.7

ethernet:
  type: LAN8720
  mdc_pin: GPIO23
  mdio_pin: GPIO18
  clk_mode: GPIO17_OUT
  phy_addr: 0
  power_pin: GPIO12

logger:

ota:

dashboard_import:
  package_import_url: github://esphome/firmware/bluetooth-proxy/olimex-esp32-poe-iso.yaml@main

external_components:
  - source: github://feens/esphome_official
    refresh: 1minutes
    components: [ esp32_ble_tracker ]

esp32_ble_tracker:
  scan_parameters:
    interval: 1100ms
    window: 1100ms
    active: true
    allowlist_address: 

      - 78:9C:85:09:72:10
      - 78:9C:85:10:89:42

bluetooth_proxy:
  active: true

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

If user exposed functionality or configuration variables are added/changed:

@codecov-commenter
Copy link

codecov-commenter commented May 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.09%. Comparing base (4d8b5ed) to head (cdfbd98).
Report is 635 commits behind head on dev.

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.
📢 Have feedback on the report? Share it here.

@@ -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) {
Copy link
Member

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

Copy link
Author

@feens feens May 17, 2024

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.

Copy link
Member

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

@feens feens force-pushed the dev branch 2 times, most recently from 9a098fb to a36b8fd Compare May 17, 2024 01:24
@@ -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
Copy link
Author

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.

Copy link
Member

@nagyrobi nagyrobi May 20, 2024

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

Copy link
Author

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

Copy link
Member

@nagyrobi nagyrobi May 21, 2024

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.

Copy link
Author

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.

Copy link
Member

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?

Copy link
Author

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.

Copy link
Member

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

Copy link
Member

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)

Copy link
Author

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_) {
Copy link
Author

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.

@feens feens force-pushed the dev branch 3 times, most recently from ca9532d to 4921b8c Compare May 17, 2024 01:38
@feens feens mentioned this pull request May 20, 2024
3 tasks
@feens feens marked this pull request as ready for review May 20, 2024 01:00
@feens
Copy link
Author

feens commented May 20, 2024

@bdraco docs changes are up, should be ready for a proper review now.

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

Successfully merging this pull request may close these issues.

None yet

5 participants