-
-
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
Ble client fixes for proxy #6596
Conversation
These checks where added to base class in esphome#5277
The disconnection event might not occure, if multiple virtual connections are active to same device
The bluetooth proxy need to inform it's controller of the fact that opening the connection failed.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #6596 +/- ##
==========================================
- Coverage 53.70% 53.43% -0.28%
==========================================
Files 50 50
Lines 9408 9537 +129
Branches 1654 1685 +31
==========================================
+ Hits 5053 5096 +43
- Misses 4056 4130 +74
- Partials 299 311 +12 ☔ View full report in Codecov by Sentry. |
error: switch has 2 consecutive identical branches [bugprone-branch-clone,-warnings-as-errors]
This all looks good to me, so I guess the question is what testing has/can be done? |
Unsure if stack will trigger CLOSE events for all type of disconnections. The samples all listen to disconnect.
Hey there @pasiz, mind taking a look at this pull request as it has been labeled with an integration ( |
I have now tested with a "broken" config like below. Where both the proxy through HA connects to the device, as well as the ESP device itself connects to the device. Seems to work now. Previously the proxy would time out it's disconnections since no disconnection even would occure. Admittedly, that is only a partial test of the changes here thou. The open failure indication i've not been able to figure out a testcase for.
|
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.
All looks good to me.
What does this implement/fix?
Types of changes
Related issue or feature (if applicable): Related to #5277 and replaces #6590
Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#
Test Environment
Example entry for
config.yaml
:# Example config.yaml
Checklist:
tests/
folder).If user exposed functionality or configuration variables are added/changed: