[ble_client] Reject descriptor_uuid combined with notify at validation time - #18109
Conversation
…n time The combination has never worked and cannot work; BLE has no descriptor notification PDU and ESP-IDF has no descriptor variant of esp_ble_gattc_register_for_notify. At runtime the descriptor handle overwrote the handle used to filter notify events, so the sensor never reached the established state. Fail configuration validation instead with a message explaining descriptors can only be polled. The descriptor_uuid constant and the validator move to the ble_client package so both the sensor and text_sensor platforms share them.
|
To use the changes from this PR as an external component, add the following to your ESPHome configuration YAML file: external_components:
- source: github://pr#18109
components: [ble_client]
refresh: 1h(Added by the PR bot) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #18109 +/- ##
=======================================
Coverage 87.25% 87.25%
=======================================
Files 64 64
Lines 14692 14692
Branches 2216 2216
=======================================
Hits 12820 12820
Misses 1565 1565
Partials 307 307 🚀 New features to boost your workflow:
|
|
|
There was a problem hiding this comment.
Tip
No blocking issues found — ready to merge.
That's kinda the point, its a nonsense combination that users shouldn't ever have. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a long-standing invalid configuration in the ble_client sensor/text_sensor platforms by rejecting descriptor_uuid used together with notify: true during config validation, preventing a runtime state where notifications never arrive.
Changes:
- Introduces a shared
validate_descriptor_not_notifyvalidator in theble_clientpackage and applies it to bothble_clientsensor and text_sensor schemas. - Moves the shared
CONF_DESCRIPTOR_UUIDconstant to theble_clientpackage so both platforms reuse the same key definition. - Adds unit tests covering the new validator behavior.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| esphome/components/ble_client/init.py | Adds shared CONF_DESCRIPTOR_UUID and a validator that rejects descriptor_uuid combined with notify. |
| esphome/components/ble_client/sensor/init.py | Reuses the shared constant/validator and applies the validator to the sensor schema. |
| esphome/components/ble_client/text_sensor/init.py | Reuses the shared constant/validator and applies the validator to the text_sensor schema. |
| tests/component_tests/ble_client/test_validation.py | Adds unit tests validating the new rejection/acceptance cases. |
| tests/component_tests/ble_client/init.py | Test package marker for the new ble_client component test directory. |
on_notify never fires without notifications either, so steering users from notify toward on_notify would trade one dead config for another. Schema level tests now feed full configs through both platform schemas so a refactor cannot silently drop the validator.
Agreed, dropping that finding. Combination never produced data at runtime. Hard failure beats silent no-op. Verified no in-tree YAML uses |
PR Review — [ble_client] Reject descriptor_uuid combined with notify at validation timeMerge-ready. Both findings from the previous round are resolved, and the one remaining note is a pre-existing footgun in adjacent code. What is solid here:
Remaining note:
✅ Resolved since last review (2)Previously-flagged issues verified fixed
🟢 Suggestions
1. [Pre-Existing Issue] `on_notify` without `notify: true` is still silently dead
|
There was a problem hiding this comment.
Tip
No blocking issues found — ready to merge.
Memory Impact AnalysisComponents:
This analysis runs automatically when components change. Memory usage is measured from a representative test configuration. |
An on_notify automation without notify true validated but never fired because nothing registered for notifications. Presence of on_notify now enables notify during validation, so the automation works and the new error message cannot steer users into a dead configuration.
|
👋 Hi there! I've automatically requested reviews from codeowners based on the files changed in this PR. @buxtronix, @clydebarrow - You've been requested to review this PR as codeowner(s) of 1 file(s) that were modified. Thanks for your time! 🙏 |
|
thanks |
What does this implement/fix?
Using
descriptor_uuidtogether withnotify: trueon a ble_client sensor or text_sensor has never worked and cannot work; BLE has no descriptor notification PDU and ESP-IDF has no descriptor variant ofesp_ble_gattc_register_for_notify, descriptors can only be read or written. At runtime the descriptor handle overwrote the handle used to filter notify events, so the sensor never reached the established state and produced no data. This was the second half of the report in #18096.Reject the combination at validation time with a message explaining that descriptors can only be polled;
on_notifyis rejected alongsidenotifysince the automation also never fires without notifications. In addition,on_notifynow impliesnotify: true; previously anon_notifyautomation withoutnotifyvalidated but never fired because nothing registered for notifications. Thedescriptor_uuidandon_notifyconstants and the validators move to the ble_client package so the sensor and text_sensor platforms share them; unit tests cover both validators and the schema wiring.Types of changes
Related issue or feature (if applicable):
Pull request in esphome.io with documentation (if applicable):
Pull request in developers.esphome.io with developer documentation (if applicable):
Test Environment
Example entry for
config.yaml:Checklist:
tests/folder).If user exposed functionality or configuration variables are added/changed: