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

fix: add retries to polling functionality #43

Merged
merged 4 commits into from
Mar 21, 2023

Conversation

Lash-L
Copy link
Collaborator

@Lash-L Lash-L commented Mar 18, 2023

this PR does two things

  1. adds a few extra logs to help debug Oral-B Since update Bluetooth blinks random during session home-assistant/core#88009

  2. potentially I am being silly here, but I think the code to get payloads was problematic, as if it failed, it would still attempt to get it as it was outside of the try catch and after finally.

I don't know if the except BleakError is worth it, as I know the connector also excepts for bleakerrors.

@codecov
Copy link

codecov bot commented Mar 18, 2023

Codecov Report

Patch coverage: 89.47% and project coverage change: -1.03 ⚠️

Comparison is base (006d94a) 96.93% compared to head (573d41d) 95.90%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #43      +/-   ##
==========================================
- Coverage   96.93%   95.90%   -1.03%     
==========================================
  Files           3        3              
  Lines         163      171       +8     
  Branches       10       11       +1     
==========================================
+ Hits          158      164       +6     
- Misses          2        4       +2     
  Partials        3        3              
Impacted Files Coverage Δ
src/oralb_ble/parser.py 94.85% <88.88%> (-1.25%) ⬇️
src/oralb_ble/__init__.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Lash-L
Copy link
Collaborator Author

Lash-L commented Mar 18, 2023

I believe we may also needs to add loggers: [] to the manifest for HA?

src/oralb_ble/parser.py Outdated Show resolved Hide resolved
@bdraco
Copy link
Member

bdraco commented Mar 20, 2023

I believe we may also needs to add loggers: [] to the manifest for HA?

👍

oral-ble should get added to loggers so when you turn on debug from the integrations screen you can see / download the logs

@bdraco bdraco self-requested a review March 20, 2023 04:24
@Lash-L
Copy link
Collaborator Author

Lash-L commented Mar 20, 2023

I believe we may also needs to add loggers: [] to the manifest for HA?

👍

oral-ble should get added to loggers so when you turn on debug from the integrations screen you can see / download the logs

Sounds good - when you're okay with me merging this in, I'll do so then I'll make a small PR to bump the version and add loggers to manifest

Let me know if there is anywhere else you think logs would be helpful for debugging

src/oralb_ble/parser.py Outdated Show resolved Hide resolved
@bdraco bdraco changed the title Move polling functionality that was outside Try finally fix: add retries to polling functionality Mar 21, 2023
@bdraco bdraco changed the title fix: add retries to polling functionality feat: add retries to polling functionality Mar 21, 2023
@bdraco bdraco changed the title feat: add retries to polling functionality fix: add retries to polling functionality Mar 21, 2023
@bdraco
Copy link
Member

bdraco commented Mar 21, 2023

Not sure if we should call this a fix or a new feature.

Probably fine either way

@bdraco
Copy link
Member

bdraco commented Mar 21, 2023

I think calling it a fix is fine here since it doesn't add new states, it only makes it less likely to fail.

@bdraco bdraco merged commit 90ab0fa into Bluetooth-Devices:main Mar 21, 2023
@bdraco
Copy link
Member

bdraco commented Mar 21, 2023

Tested all good 👍

@Lash-L
Copy link
Collaborator Author

Lash-L commented Mar 22, 2023

Perfect, thanks @bdraco

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

Successfully merging this pull request may close these issues.

2 participants